[PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

Hi Russell,

this series should apply on linux-arm git repository, on branch
clearfog.

Some internet providers are already starting to offer 2.5G copper
connectivity to their users. On Turris Omnia the SFP port is capable
of 2.5G speed, so we tested some copper SFP modules.

This adds support to the SFP subsystem for 10G RollBall copper modules
which contain a Marvell 88X3310 PHY. By default these modules are
configured in 10GKR only mode on the host interface, and also contain
some bad information in EEPROM (the extended_cc byte).

The PHY in these modules is also accessed via a different I2C protocol
than the standard one.

Patch 1 adds support for this different I2C MDIO bus.
Patch 2 adds support for these modules into the SFP driver.
Patch 3 adds support for chaning MACTYPE in marvell10g PHY driver so
that the PHY will connect to the host interface on Turris Omnia even
though it only supports SGMII/1000base-x/2500base-x modes.
Patch 4 changes phylink code so that a PHY can be attached even though
802.3z mode is requested.

Marek

Marek Behún (4):
  net: phy: add I2C mdio bus for RollBall compatible SFPs
  net: phy: sfp: add support for multigig RollBall modules
  net: phy: marvell10g: change MACTYPE according to phydev->interface
  net: phylink: don't fail attaching phy on 1000base-x/2500base-x mode

 drivers/net/phy/Makefile            |   2 +-
 drivers/net/phy/marvell10g.c        |  32 +++-
 drivers/net/phy/mdio-i2c-rollball.c | 238 ++++++++++++++++++++++++++++
 drivers/net/phy/mdio-i2c.h          |   1 +
 drivers/net/phy/phylink.c           |   4 +-
 drivers/net/phy/sfp.c               |  62 ++++++--
 include/linux/sfp.h                 |   5 +
 7 files changed, 322 insertions(+), 22 deletions(-)
 create mode 100644 drivers/net/phy/mdio-i2c-rollball.c

-- 
2.26.2

[PATCH RFC russell-king 1/4] net: phy: add I2C mdio bus for RollBall compatible SFPs

[Marek Behún]

Some multigig SFPs from RollBall and Hilink do not expose functional
MDIO access to the internal PHY of the SFP via I2C address 0x56
(although there seems to be read-only clause 22 access on this address).

Instead these SFPs PHY can be accessed via I2C via the SFP Enhanced
Digital Diagnostic Interface - I2C address 0x51.

This driver adds support for this as a MDIO bus.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/Makefile            |   2 +-
 drivers/net/phy/mdio-i2c-rollball.c | 238 ++++++++++++++++++++++++++++
 drivers/net/phy/mdio-i2c.h          |   1 +
 drivers/net/phy/sfp.c               |   5 -
 include/linux/sfp.h                 |   5 +
 5 files changed, 245 insertions(+), 6 deletions(-)
 create mode 100644 drivers/net/phy/mdio-i2c-rollball.c

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 4500050faf64f..ce12d5bf02b1e 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -39,7 +39,7 @@ obj-$(CONFIG_MDIO_BUS_MUX_MULTIPLEXER) += mdio-mux-multiplexer.o
 obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
 obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
 obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
-obj-$(CONFIG_MDIO_I2C)		+= mdio-i2c.o
+obj-$(CONFIG_MDIO_I2C)		+= mdio-i2c.o mdio-i2c-rollball.o
 obj-$(CONFIG_MDIO_IPQ4019)	+= mdio-ipq4019.o
 obj-$(CONFIG_MDIO_IPQ8064)	+= mdio-ipq8064.o
 obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
diff --git a/drivers/net/phy/mdio-i2c-rollball.c b/drivers/net/phy/mdio-i2c-rollball.c
new file mode 100644
index 0000000000000..8355d19fe4192
--- /dev/null
+++ b/drivers/net/phy/mdio-i2c-rollball.c
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: GPL-2.0
+/* MDIO I2C bridge for RollBall compatible SFPs
+ *
+ * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz>
+ *
+ * RollBall compatible SFPs expose their internal PHY in a different way
+ * from the one handled by mdio-i2c.c. This driver exposes interface for
+ * this RollBall interface.
+ */
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/phy.h>
+#include <linux/sfp.h>
+
+#include "mdio-i2c.h"
+
+/* RollBall SFPs do not access internal PHY via I2C address 0x56, but
+ * instead via address 0x51, when SFP page is set to 0x03 and password to
+ * 0xffffffff:
+ *
+ * address  size  contents  description
+ * -------  ----  --------  -----------
+ * 0x80     1     CMD       0x01/0x02/0x04 for write/read/done
+ * 0x81     1     DEV       Clause 45 device
+ * 0x82     2     REG       Clause 45 register
+ * 0x84     2     VAL       Register value
+ */
+#define SFP_DIAG_I2C_ADDR		0x51
+
+#define ROLLBALL_SFP_PASSWORD_ADDR	0x7b
+
+#define ROLLBALL_SFP_MDIO_PAGE		0x03
+
+#define ROLLBALL_CMD_ADDR		0x80
+#define ROLLBALL_DATA_ADDR		0x81
+
+#define ROLLBALL_CMD_WRITE		0x01
+#define ROLLBALL_CMD_READ		0x02
+#define ROLLBALL_CMD_DONE		0x04
+
+static int i2c_rollball_mii_poll(struct mii_bus *bus, u8 *buf, size_t len)
+{
+	struct i2c_adapter *i2c = bus->priv;
+	struct i2c_msg msgs[2];
+	u8 buf0[2], *res;
+	int i, ret;
+
+	buf0[0] = ROLLBALL_CMD_ADDR;
+
+	msgs[0].addr = SFP_DIAG_I2C_ADDR;
+	msgs[0].flags = 0;
+	msgs[0].len = 1;
+	msgs[0].buf = &buf0[0];
+
+	res = buf ? buf : &buf0[1];
+
+	msgs[1].addr = SFP_DIAG_I2C_ADDR;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].len = buf ? len : 1;
+	msgs[1].buf = res;
+
+	/* It takes up to 70 ms to access a register for these SFPs. */
+	i = 10;
+	do {
+		msleep(20);
+
+		ret = i2c_transfer(i2c, msgs, ARRAY_SIZE(msgs));
+		if (ret < 0)
+			return ret;
+		else if (ret != ARRAY_SIZE(msgs))
+			return -EIO;
+
+		if (*res == ROLLBALL_CMD_DONE)
+			return 0;
+	} while (i-- > 0);
+
+	dev_dbg(&bus->dev, "poll timed out\n");
+
+	return -ETIMEDOUT;
+}
+
+static int i2c_rollball_mii_cmd(struct mii_bus *bus, u8 cmd, u8 *data, size_t len)
+{
+	struct i2c_adapter *i2c = bus->priv;
+	struct i2c_msg msgs[2];
+	u8 cmdbuf[2];
+	int ret;
+
+	msgs[0].addr = SFP_DIAG_I2C_ADDR;
+	msgs[0].flags = 0;
+	msgs[0].len = len;
+	msgs[0].buf = data;
+
+	cmdbuf[0] = ROLLBALL_CMD_ADDR;
+	cmdbuf[1] = cmd;
+
+	msgs[1].addr = SFP_DIAG_I2C_ADDR;
+	msgs[1].flags = 0;
+	msgs[1].len = sizeof(cmdbuf);
+	msgs[1].buf = cmdbuf;
+
+	ret = i2c_transfer(i2c, msgs, 2);
+	if (ret < 0)
+		return ret;
+
+	return ret == ARRAY_SIZE(msgs) ? 0 : -EIO;
+}
+
+static int i2c_rollball_mii_read(struct mii_bus *bus, int phy_id, int reg)
+{
+	u8 buf[4], res[6];
+	u16 val;
+	int ret;
+
+	if (phy_id != SFP_PHY_ADDR)
+		return 0xffff;
+
+	if (!(reg & MII_ADDR_C45))
+		return -EOPNOTSUPP;
+
+	buf[0] = ROLLBALL_DATA_ADDR;
+	buf[1] = (reg >> 16) & 0x1f;
+	buf[2] = (reg >> 8) & 0xff;
+	buf[3] = reg & 0xff;
+
+	ret = i2c_rollball_mii_cmd(bus, ROLLBALL_CMD_READ, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_rollball_mii_poll(bus, res, sizeof(res));
+	if (ret == -ETIMEDOUT)
+		return 0xffff;
+	else if (ret < 0)
+		return ret;
+
+	val = res[4];
+	val <<= 8;
+	val |= res[5];
+
+	dev_dbg(&bus->dev, "read reg %02x:%04x = %04x\n", (reg >> 16) & 0x1f, reg & 0xffff, val);
+
+	return val;
+}
+
+static int i2c_rollball_mii_write(struct mii_bus *bus, int phy_id, int reg, u16 val)
+{
+	u8 buf[6];
+	int ret;
+
+	if (phy_id != SFP_PHY_ADDR)
+		return 0;
+
+	if (!(reg & MII_ADDR_C45))
+		return -EOPNOTSUPP;
+
+	buf[0] = ROLLBALL_DATA_ADDR;
+	buf[1] = (reg >> 16) & 0x1f;
+	buf[2] = (reg >> 8) & 0xff;
+	buf[3] = reg & 0xff;
+	buf[4] = val >> 8;
+	buf[5] = val & 0xff;
+
+	ret = i2c_rollball_mii_cmd(bus, ROLLBALL_CMD_WRITE, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_rollball_mii_poll(bus, NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&bus->dev, "write reg %02x:%04x = %04x\n", (reg >> 16) & 0x1f, reg & 0xffff, val);
+
+	return 0;
+}
+
+static int i2c_rollball_init(struct i2c_adapter *i2c)
+{
+	struct i2c_msg msgs[2];
+	u8 page[2], password[5];
+	int ret;
+
+	page[0] = SFP_PAGE;
+	page[1] = ROLLBALL_SFP_MDIO_PAGE;
+
+	msgs[0].addr = SFP_DIAG_I2C_ADDR;
+	msgs[0].flags = 0;
+	msgs[0].len = sizeof(page);
+	msgs[0].buf = page;
+
+	password[0] = ROLLBALL_SFP_PASSWORD_ADDR;
+	password[1] = 0xff;
+	password[2] = 0xff;
+	password[3] = 0xff;
+	password[4] = 0xff;
+
+	msgs[1].addr = SFP_DIAG_I2C_ADDR;
+	msgs[1].flags = 0;
+	msgs[1].len = sizeof(password);
+	msgs[1].buf = password;
+
+	ret = i2c_transfer(i2c, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0)
+		return ret;
+
+	return ret == ARRAY_SIZE(msgs) ? 0 : -EIO;
+}
+
+struct mii_bus *mdio_i2c_rollball_alloc(struct device *parent, struct i2c_adapter *i2c)
+{
+	struct mii_bus *mii;
+	int ret;
+
+	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
+		return ERR_PTR(-EINVAL);
+
+	mii = mdiobus_alloc();
+	if (!mii)
+		return ERR_PTR(-ENOMEM);
+
+	ret = i2c_rollball_init(i2c);
+	if (ret < 0) {
+		dev_err(parent, "cannot initialize SFP for MDIO access\n");
+		return ERR_PTR(ret);
+	}
+
+	snprintf(mii->id, MII_BUS_ID_SIZE, "i2c-rollball:%s", dev_name(parent));
+	mii->parent = parent;
+	mii->read = i2c_rollball_mii_read;
+	mii->write = i2c_rollball_mii_write;
+	mii->priv = i2c;
+
+	return mii;
+}
+EXPORT_SYMBOL_GPL(mdio_i2c_rollball_alloc);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MDIO I2C bridge library for RollBall compatible SFPs");
+MODULE_AUTHOR("Marek Behun <marek.behun@nic.cz>");
diff --git a/drivers/net/phy/mdio-i2c.h b/drivers/net/phy/mdio-i2c.h
index b1d27f7cd23fb..d96fcfa9637e0 100644
--- a/drivers/net/phy/mdio-i2c.h
+++ b/drivers/net/phy/mdio-i2c.h
@@ -12,5 +12,6 @@ struct i2c_adapter;
 struct mii_bus;
 
 struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c);
+struct mii_bus *mdio_i2c_rollball_alloc(struct device *parent, struct i2c_adapter *i2c);
 
 #endif
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index c24b0e83dd329..a62fa2e5ae4e6 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -202,11 +202,6 @@ static const enum gpiod_flags gpio_flags[] = {
 #define T_PROBE_RETRY_SLOW	msecs_to_jiffies(5000)
 #define R_PROBE_RETRY_SLOW	12
 
-/* SFP modules appear to always have their PHY configured for bus address
- * 0x56 (which with mdio-i2c, translates to a PHY address of 22).
- */
-#define SFP_PHY_ADDR	22
-
 struct sff_data {
 	unsigned int gpios;
 	bool (*module_supported)(const struct sfp_eeprom_id *id);
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index 2da1a5181779e..035b665ca702e 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -3,6 +3,11 @@
 
 #include <linux/phy.h>
 
+/* SFP modules appear to always have their PHY configured for bus address
+ * 0x56 (which with mdio-i2c, translates to a PHY address of 22).
+ */
+#define SFP_PHY_ADDR	22
+
 struct sfp_eeprom_base {
 	u8 phys_id;
 	u8 phys_ext_id;
-- 
2.26.2

[PATCH RFC russell-king 2/4] net: phy: sfp: add support for multigig RollBall modules

[Marek Behún]

This adds support for multigig copper SFP modules from RollBall/Hilink.
These modules have a specific way to access clause 45 registers of the
internal PHY.

We also need to wait at least 25 seconds after deasserting TX disable
before accessing the PHY. The code waits for 30 seconds just to be sure.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/sfp.c | 57 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index a62fa2e5ae4e6..fe72282e96c7d 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -167,6 +167,7 @@ static const enum gpiod_flags gpio_flags[] = {
 #define T_WAIT			msecs_to_jiffies(50)
 #define T_START_UP		msecs_to_jiffies(300)
 #define T_START_UP_BAD_GPON	msecs_to_jiffies(60000)
+#define T_START_UP_LONG_PHY	msecs_to_jiffies(30000)
 
 /* t_reset is the time required to assert the TX_DISABLE signal to reset
  * an indicated TX_FAULT.
@@ -243,6 +244,7 @@ struct sfp {
 	struct sfp_eeprom_id id;
 	unsigned int module_power_mW;
 	unsigned int module_t_start_up;
+	bool rollball_mii;
 
 #if IS_ENABLED(CONFIG_HWMON)
 	struct sfp_diag diag;
@@ -394,9 +396,6 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 
 static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
 {
-	struct mii_bus *i2c_mii;
-	int ret;
-
 	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
 		return -EINVAL;
 
@@ -404,7 +403,19 @@ static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
 	sfp->read = sfp_i2c_read;
 	sfp->write = sfp_i2c_write;
 
-	i2c_mii = mdio_i2c_alloc(sfp->dev, i2c);
+	return 0;
+}
+
+static int sfp_i2c_mii_probe(struct sfp *sfp)
+{
+	struct mii_bus *i2c_mii;
+	int ret;
+
+	if (sfp->rollball_mii)
+		i2c_mii = mdio_i2c_rollball_alloc(sfp->dev, sfp->i2c);
+	else
+		i2c_mii = mdio_i2c_alloc(sfp->dev, sfp->i2c);
+
 	if (IS_ERR(i2c_mii))
 		return PTR_ERR(i2c_mii);
 
@@ -422,6 +433,14 @@ static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
 	return 0;
 }
 
+static void sfp_i2c_mii_remove(struct sfp *sfp)
+{
+	if (sfp->i2c_mii) {
+		mdiobus_unregister(sfp->i2c_mii);
+		mdiobus_free(sfp->i2c_mii);
+	}
+}
+
 /* Interface */
 static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
 {
@@ -1419,6 +1438,7 @@ static void sfp_sm_phy_detach(struct sfp *sfp)
 	phy_device_remove(sfp->mod_phy);
 	phy_device_free(sfp->mod_phy);
 	sfp->mod_phy = NULL;
+	sfp_i2c_mii_remove(sfp);
 }
 
 static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
@@ -1426,10 +1446,17 @@ static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
 	struct phy_device *phy;
 	int err;
 
+	err = sfp_i2c_mii_probe(sfp);
+	if (err)
+		return err;
+
 	phy = get_phy_device(sfp->i2c_mii, SFP_PHY_ADDR, is_c45);
-	if (phy == ERR_PTR(-ENODEV))
+	if (phy == ERR_PTR(-ENODEV)) {
+		sfp_i2c_mii_remove(sfp);
 		return PTR_ERR(phy);
+	}
 	if (IS_ERR(phy)) {
+		sfp_i2c_mii_remove(sfp);
 		dev_err(sfp->dev, "mdiobus scan returned %ld\n", PTR_ERR(phy));
 		return PTR_ERR(phy);
 	}
@@ -1437,6 +1464,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
 	err = phy_device_register(phy);
 	if (err) {
 		phy_device_free(phy);
+		sfp_i2c_mii_remove(sfp);
 		dev_err(sfp->dev, "phy_device_register failed: %d\n", err);
 		return err;
 	}
@@ -1445,6 +1473,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
 	if (err) {
 		phy_device_remove(phy);
 		phy_device_free(phy);
+		sfp_i2c_mii_remove(sfp);
 		dev_err(sfp->dev, "sfp_add_phy failed: %d\n", err);
 		return err;
 	}
@@ -1665,6 +1694,7 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	struct sfp_eeprom_id id;
 	bool cotsworks_sfbg;
 	bool cotsworks;
+	bool rollball;
 	u8 check;
 	int ret;
 
@@ -1730,7 +1760,17 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 		}
 	}
 
+	rollball = (!memcmp(id.base.vendor_name, "OEM             ", 16) &&
+		    (!memcmp(id.base.vendor_pn, "SFP-10G-T       ", 16) ||
+		     !memcmp(id.base.vendor_pn, "RTSFP-10        ", 16) ||
+		     !memcmp(id.base.vendor_pn, "RTSFP-2.5G      ", 16)));
+	if (rollball) {
+		/* TODO: try to write this to EEPROM */
+		id.base.extended_cc = SFF8024_ECC_10GBASE_T_SFI;
+	}
+
 	sfp->id = id;
+	sfp->rollball_mii = rollball;
 
 	dev_info(sfp->dev, "module %.*s %.*s rev %.*s sn %.*s dc %.*s\n",
 		 (int)sizeof(id.base.vendor_name), id.base.vendor_name,
@@ -1760,6 +1800,8 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	if (!memcmp(id.base.vendor_name, "ALCATELLUCENT   ", 16) &&
 	    !memcmp(id.base.vendor_pn, "3FE46541AA      ", 16))
 		sfp->module_t_start_up = T_START_UP_BAD_GPON;
+	else if (rollball)
+		sfp->module_t_start_up = T_START_UP_LONG_PHY;
 	else
 		sfp->module_t_start_up = T_START_UP;
 
@@ -2264,10 +2306,7 @@ static void sfp_cleanup(void *data)
 
 	cancel_delayed_work_sync(&sfp->poll);
 	cancel_delayed_work_sync(&sfp->timeout);
-	if (sfp->i2c_mii) {
-		mdiobus_unregister(sfp->i2c_mii);
-		mdiobus_free(sfp->i2c_mii);
-	}
+	sfp_i2c_mii_remove(sfp);
 	if (sfp->i2c)
 		i2c_put_adapter(sfp->i2c);
 	kfree(sfp);
-- 
2.26.2

Re: [PATCH RFC russell-king 2/4] net: phy: sfp: add support for multigig RollBall modules

[Russell King - ARM Linux admin]

On Tue, Aug 11, 2020 at 12:06:43AM +0200, Marek Behún wrote:
> This adds support for multigig copper SFP modules from RollBall/Hilink.
> These modules have a specific way to access clause 45 registers of the
> internal PHY.
> 
> We also need to wait at least 25 seconds after deasserting TX disable
> before accessing the PHY. The code waits for 30 seconds just to be sure.

I wonder why it takes so long - the 88x3310 boots very quickly.

> +static int sfp_i2c_mii_probe(struct sfp *sfp)
> +{
> +	struct mii_bus *i2c_mii;
> +	int ret;
> +
> +	if (sfp->rollball_mii)
> +		i2c_mii = mdio_i2c_rollball_alloc(sfp->dev, sfp->i2c);
> +	else
> +		i2c_mii = mdio_i2c_alloc(sfp->dev, sfp->i2c);
> +

I think I'd prefer to find a way to teach the existing mdio_i2c code
about different protocols rather than having a load of different buses
that we have to select between.  Maybe we can do that via the PHY
address, or maybe we should have a call into mdio_i2c that tells it
which protocol should be used?

The reason I don't like this approach is it seems to me to be very
heavy handed... using a sledge hammer to crack a nut.

> +	rollball = (!memcmp(id.base.vendor_name, "OEM             ", 16) &&
> +		    (!memcmp(id.base.vendor_pn, "SFP-10G-T       ", 16) ||
> +		     !memcmp(id.base.vendor_pn, "RTSFP-10        ", 16) ||
> +		     !memcmp(id.base.vendor_pn, "RTSFP-2.5G      ", 16)));
> +	if (rollball) {
> +		/* TODO: try to write this to EEPROM */
> +		id.base.extended_cc = SFF8024_ECC_10GBASE_T_SFI;

Should we really be "fixing" vendors EEPROMs for them?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 2/4] net: phy: sfp: add support for multigig RollBall modules

[Marek Behún]

On Tue, 11 Aug 2020 16:15:53 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> > +	if (rollball) {
> > +		/* TODO: try to write this to EEPROM */
> > +		id.base.extended_cc = SFF8024_ECC_10GBASE_T_SFI;  
> 
> Should we really be "fixing" vendors EEPROMs for them?
> 

Are you reffering to the TODO comment or the id.base.extended_cc
assignment?
If the comment, well, your code does it for cotsworks modules, but I am
actually indifferent.
If the assignment - either this needs to be done or the
sfp_probe_for_phy and sfp_may_have_phy functions have to be changed so
that they check for rollball module by vendor ID.

Marek

Re: [PATCH RFC russell-king 2/4] net: phy: sfp: add support for multigig RollBall modules

[Russell King - ARM Linux admin]

On Wed, Aug 12, 2020 at 03:33:26PM +0200, Marek Behún wrote:
> On Tue, 11 Aug 2020 16:15:53 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > > +	if (rollball) {
> > > +		/* TODO: try to write this to EEPROM */
> > > +		id.base.extended_cc = SFF8024_ECC_10GBASE_T_SFI;  
> > 
> > Should we really be "fixing" vendors EEPROMs for them?
> > 
> 
> Are you reffering to the TODO comment or the id.base.extended_cc
> assignment?
> If the comment, well, your code does it for cotsworks modules, but I am
> actually indifferent.

No, that's Chris' code, and there's quite a bit of history there:
It appears Cotsworks programmed things like the serial number into
the EEPROM and did not update the checksums.  After quite some time,
it seems Cotsworks have seen sense, and have fixed their production
line to properly program the EEPROM, but that leaves a whole bunch
of modules with bad checksums.

I'm more than happy that we should continue issuing the warning, but
Chris has decided to fix them up.  I'm not particularly happy with
that idea, but I didn't get the chance to express it before David
picked up the patch.  So, it's now in mainline.

Fixing the checksum for a module that is known to suffer bad checksums
is one thing - it's a single byte write, and as the checksum is wrong,
it's likely other systems that know about the issue will ignore it.

However, changing the module description to be "correct" is a completely
different level - there are many modules that do not report "correct"
data, and, if we start fixing these up, it's likely that fixups that
other SFP cage implementations have could stop working since they may
not recognise the module.

Remember, things like the extended CC codes are dependent on the SFF
spec revisions, so if we start changing the extended CC code in byte
36, should we also change the SFF8472 compliance code as well (to
be > rev 11.9)?  Since SFF8472 rev 11.9 changed the definition of this
byte.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 2/4] net: phy: sfp: add support for multigig RollBall modules

[Marek Behún]

On Wed, 12 Aug 2020 15:33:04 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 03:33:26PM +0200, Marek Behún wrote:
> > On Tue, 11 Aug 2020 16:15:53 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> >   
> > > > +	if (rollball) {
> > > > +		/* TODO: try to write this to EEPROM */
> > > > +		id.base.extended_cc =
> > > > SFF8024_ECC_10GBASE_T_SFI;    
> > > 
> > > Should we really be "fixing" vendors EEPROMs for them?
> > >   
> > 
> > Are you reffering to the TODO comment or the id.base.extended_cc
> > assignment?
> > If the comment, well, your code does it for cotsworks modules, but
> > I am actually indifferent.  
> 
> No, that's Chris' code, and there's quite a bit of history there:
> It appears Cotsworks programmed things like the serial number into
> the EEPROM and did not update the checksums.  After quite some time,
> it seems Cotsworks have seen sense, and have fixed their production
> line to properly program the EEPROM, but that leaves a whole bunch
> of modules with bad checksums.
> 
> I'm more than happy that we should continue issuing the warning, but
> Chris has decided to fix them up.  I'm not particularly happy with
> that idea, but I didn't get the chance to express it before David
> picked up the patch.  So, it's now in mainline.
> 
> Fixing the checksum for a module that is known to suffer bad checksums
> is one thing - it's a single byte write, and as the checksum is wrong,
> it's likely other systems that know about the issue will ignore it.
> 
> However, changing the module description to be "correct" is a
> completely different level - there are many modules that do not
> report "correct" data, and, if we start fixing these up, it's likely
> that fixups that other SFP cage implementations have could stop
> working since they may not recognise the module.
> 
> Remember, things like the extended CC codes are dependent on the SFF
> spec revisions, so if we start changing the extended CC code in byte
> 36, should we also change the SFF8472 compliance code as well (to
> be > rev 11.9)?  Since SFF8472 rev 11.9 changed the definition of this
> byte.
> 

Thank you Russell for this explanation.

[PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

RollBall SFPs contain Marvell 88X3310 PHY, but they have configuration
pins strapped so that MACTYPE is configured in XFI with Rate Matching
mode.

When these SFPs are inserted into a device which only supports lower
speeds on host interface, we need to configure the MACTYPE to a mode
in which the H unit changes SerDes speed according to speed on the
copper interface. I chose to use the
10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/marvell10g.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 29575442b25b2..13a588fa69e77 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -81,6 +81,7 @@ enum {
 	MV_V2_PORT_CTRL_SWRST	= BIT(15),
 	MV_V2_PORT_CTRL_PWRDOWN = BIT(11),
 	MV_V2_PORT_MAC_TYPE_MASK = 0x7,
+	MV_V2_PORT_MAC_TYPE_10GBR_SGMII_AN = 0x4,
 	MV_V2_PORT_MAC_TYPE_RATE_MATCH = 0x6,
 	/* Temperature control/read registers (88X3310 only) */
 	MV_V2_TEMP_CTRL		= 0xf08a,
@@ -258,17 +259,40 @@ static int mv3310_power_down(struct phy_device *phydev)
 static int mv3310_power_up(struct phy_device *phydev)
 {
 	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	u16 val, mask;
 	int ret;
 
 	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
 				 MV_V2_PORT_CTRL_PWRDOWN);
 
-	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310 ||
-	    priv->firmware_ver < 0x00030000)
+	if (ret < 0)
 		return ret;
 
-	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
-				MV_V2_PORT_CTRL_SWRST);
+	/* TODO: add support for changing MACTYPE on 88E2110 via register 1.C0A4.2:0 */
+	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310)
+		return 0;
+
+	val = mask = MV_V2_PORT_CTRL_SWRST;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		val |= MV_V2_PORT_MAC_TYPE_10GBR_SGMII_AN;
+		mask |= MV_V2_PORT_MAC_TYPE_MASK;
+		break;
+
+	default:
+		/* Otherwise we assume that the MACTYPE is set correctly by strapping pins.
+		 * Feel free to add support for changing MACTYPE for other modes
+		 * (XAUI/RXAUI/USXGMII).
+		 */
+
+		/* reset is not needed for firmware version < 0.3.0.0 when not changing MACTYPE */
+		if (priv->firmware_ver < 0x00030000)
+			return 0;
+	}
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, mask, val);
 }
 
 static int mv3310_reset(struct phy_device *phydev, u32 unit)
-- 
2.26.2

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Russell King - ARM Linux admin]

On Tue, Aug 11, 2020 at 12:06:44AM +0200, Marek Behún wrote:
> RollBall SFPs contain Marvell 88X3310 PHY, but they have configuration
> pins strapped so that MACTYPE is configured in XFI with Rate Matching
> mode.
> 
> When these SFPs are inserted into a device which only supports lower
> speeds on host interface, we need to configure the MACTYPE to a mode
> in which the H unit changes SerDes speed according to speed on the
> copper interface. I chose to use the
> 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode.

We actually need to have more inteligence in the driver, since we
actually assume that it is in the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII
mode without really checking.

Note that there are differences in the way the mactype field is
interpreted depending on exactly what chip we have.  For example,
3310 and 3340 are different.  That said, I've not heard of anyone
using the 3340 yet.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

On Tue, 11 Aug 2020 16:21:44 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Tue, Aug 11, 2020 at 12:06:44AM +0200, Marek Behún wrote:
> > RollBall SFPs contain Marvell 88X3310 PHY, but they have
> > configuration pins strapped so that MACTYPE is configured in XFI
> > with Rate Matching mode.
> > 
> > When these SFPs are inserted into a device which only supports lower
> > speeds on host interface, we need to configure the MACTYPE to a mode
> > in which the H unit changes SerDes speed according to speed on the
> > copper interface. I chose to use the
> > 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode.  
> 
> We actually need to have more inteligence in the driver, since we
> actually assume that it is in the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII
> mode without really checking.
> 
> Note that there are differences in the way the mactype field is
> interpreted depending on exactly what chip we have.  For example,
> 3310 and 3340 are different.  That said, I've not heard of anyone
> using the 3340 yet.
> 

Russell, I am aware that MACTYPE modes are interpreted differently for
3310 vs 3340, but this only affects modes 0-3, which the driver does
not check for even after applying my patch.

There is another problem though: I think the PHY driver, when deciding
whether to set MACTYPE from the XFI with rate matching mode to the
10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode, should check which
modes the underlying MAC support.

If the underlying MAC supports only XFI mode, than the MACTYPE should
be set to XFI with rate matching. But on Omnia for example the MAC
supports SGMII/1000base-s/2500base-x, so on Omnia the MACTYPE should be
changed.

Currently this information is given in your repository by the mvneta
driver to phylink in the call to phylink_create. But there is no way
for the PHY driver to get this information from phylink currently, and
even if phylink exposed a function to return the config member of
struct phylink, the problem is that at the time when mv3310_power_up is
called, the phydev->phylink is not yet set (this is done in
phylink_bringup_phy, and mv3310_power_up is called sometime in the
phylink_attach_phy).

Marek

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Russell King - ARM Linux admin]

On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote:
> There is another problem though: I think the PHY driver, when deciding
> whether to set MACTYPE from the XFI with rate matching mode to the
> 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode, should check which
> modes the underlying MAC support.

I'm aware of that problem.  I have some experimental patches which add
PHY interface mode bitmaps to the MAC, PHY, and SFP module parsing
functions.  I have stumbled on some problems though - it's going to be
another API change (and people are already whinging about the phylink
API changing "too quickly", were too quickly seems to be defined as
once in three years), and in some cases, DSA, it's extremely hard to
work out how to properly set such a bitmap due to DSA's layered
approach.

Having bitmaps means that we can take the union of what the MAC and
PHY supports, and decide which MACTYPE setting would be most suitable.
However, to do that we're into also changing phylib's interfaces as
well.

> driver to phylink in the call to phylink_create. But there is no way
> for the PHY driver to get this information from phylink currently, and
> even if phylink exposed a function to return the config member of
> struct phylink, the problem is that at the time when mv3310_power_up is
> called, the phydev->phylink is not yet set (this is done in
> phylink_bringup_phy, and mv3310_power_up is called sometime in the
> phylink_attach_phy).

We _really_ do not want phylib calling back into phylink functions.
That would tie phylink functionality into phylib and cause problems
when phylink is not being used.

I would prefer phylib to be passed "the MAC can use these interface
types, and would prefer to use this interface type" and have the
phylib layer (along with the phylib driver) make the decision about
which mode should be used.  That also means that non-phylink MACs
can also use it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

On Wed, 12 Aug 2020 16:00:54 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote:
> > There is another problem though: I think the PHY driver, when
> > deciding whether to set MACTYPE from the XFI with rate matching
> > mode to the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode,
> > should check which modes the underlying MAC support.  
> 
> I'm aware of that problem.  I have some experimental patches which add
> PHY interface mode bitmaps to the MAC, PHY, and SFP module parsing
> functions.  I have stumbled on some problems though - it's going to be
> another API change (and people are already whinging about the phylink
> API changing "too quickly", were too quickly seems to be defined as
> once in three years), and in some cases, DSA, it's extremely hard to
> work out how to properly set such a bitmap due to DSA's layered
> approach.
> 

If by your experimental patches you mean
  net: mvneta: fill in phy interface mode bitmap
  net: mvpp2: fill in phy interface mode bitmap
found here
  http://git.arm.linux.org.uk/cgit/linux-arm.git/log/?h=clearfog
I am currently working on top of them.

> Having bitmaps means that we can take the union of what the MAC and
> PHY supports, and decide which MACTYPE setting would be most suitable.
> However, to do that we're into also changing phylib's interfaces as
> well.
> 
> > driver to phylink in the call to phylink_create. But there is no way
> > for the PHY driver to get this information from phylink currently,
> > and even if phylink exposed a function to return the config member
> > of struct phylink, the problem is that at the time when
> > mv3310_power_up is called, the phydev->phylink is not yet set (this
> > is done in phylink_bringup_phy, and mv3310_power_up is called
> > sometime in the phylink_attach_phy).  
> 
> We _really_ do not want phylib calling back into phylink functions.
> That would tie phylink functionality into phylib and cause problems
> when phylink is not being used.
> 
> I would prefer phylib to be passed "the MAC can use these interface
> types, and would prefer to use this interface type" and have the
> phylib layer (along with the phylib driver) make the decision about
> which mode should be used.  That also means that non-phylink MACs
> can also use it.
> 

I may try to propose something, but in the meantime do you think the
current version of the patch
  net: phy: marvell10g: change MACTYPE according to phydev->interface
is acceptable?

Marek

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Russell King - ARM Linux admin]

On Wed, Aug 12, 2020 at 05:37:16PM +0200, Marek Behún wrote:
> On Wed, 12 Aug 2020 16:00:54 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote:
> > > There is another problem though: I think the PHY driver, when
> > > deciding whether to set MACTYPE from the XFI with rate matching
> > > mode to the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode,
> > > should check which modes the underlying MAC support.  
> > 
> > I'm aware of that problem.  I have some experimental patches which add
> > PHY interface mode bitmaps to the MAC, PHY, and SFP module parsing
> > functions.  I have stumbled on some problems though - it's going to be
> > another API change (and people are already whinging about the phylink
> > API changing "too quickly", were too quickly seems to be defined as
> > once in three years), and in some cases, DSA, it's extremely hard to
> > work out how to properly set such a bitmap due to DSA's layered
> > approach.
> > 
> 
> If by your experimental patches you mean
>   net: mvneta: fill in phy interface mode bitmap
>   net: mvpp2: fill in phy interface mode bitmap
> found here
>   http://git.arm.linux.org.uk/cgit/linux-arm.git/log/?h=clearfog
> I am currently working on top of them.
> 
> > Having bitmaps means that we can take the union of what the MAC and
> > PHY supports, and decide which MACTYPE setting would be most suitable.
> > However, to do that we're into also changing phylib's interfaces as
> > well.
> > 
> > > driver to phylink in the call to phylink_create. But there is no way
> > > for the PHY driver to get this information from phylink currently,
> > > and even if phylink exposed a function to return the config member
> > > of struct phylink, the problem is that at the time when
> > > mv3310_power_up is called, the phydev->phylink is not yet set (this
> > > is done in phylink_bringup_phy, and mv3310_power_up is called
> > > sometime in the phylink_attach_phy).  
> > 
> > We _really_ do not want phylib calling back into phylink functions.
> > That would tie phylink functionality into phylib and cause problems
> > when phylink is not being used.
> > 
> > I would prefer phylib to be passed "the MAC can use these interface
> > types, and would prefer to use this interface type" and have the
> > phylib layer (along with the phylib driver) make the decision about
> > which mode should be used.  That also means that non-phylink MACs
> > can also use it.
> > 
> 
> I may try to propose something, but in the meantime do you think the
> current version of the patch
>   net: phy: marvell10g: change MACTYPE according to phydev->interface
> is acceptable?

Well, I have other questions about it.  Why are you doing it in
the power_up function?  Do you find that the MACTYPE field is
lost when clearing the power down bit?  From what I read, it should
only change on hardware reset, and we don't hardware reset when we
come out of power down - only software reset.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

On Wed, 12 Aug 2020 16:48:37 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 05:37:16PM +0200, Marek Behún wrote:
> > On Wed, 12 Aug 2020 16:00:54 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> >   
> > > On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote:  
> > > > There is another problem though: I think the PHY driver, when
> > > > deciding whether to set MACTYPE from the XFI with rate matching
> > > > mode to the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode,
> > > > should check which modes the underlying MAC support.    
> > > 
> > > I'm aware of that problem.  I have some experimental patches
> > > which add PHY interface mode bitmaps to the MAC, PHY, and SFP
> > > module parsing functions.  I have stumbled on some problems
> > > though - it's going to be another API change (and people are
> > > already whinging about the phylink API changing "too quickly",
> > > were too quickly seems to be defined as once in three years), and
> > > in some cases, DSA, it's extremely hard to work out how to
> > > properly set such a bitmap due to DSA's layered approach.
> > >   
> > 
> > If by your experimental patches you mean
> >   net: mvneta: fill in phy interface mode bitmap
> >   net: mvpp2: fill in phy interface mode bitmap
> > found here
> >   http://git.arm.linux.org.uk/cgit/linux-arm.git/log/?h=clearfog
> > I am currently working on top of them.
> >   
> > > Having bitmaps means that we can take the union of what the MAC
> > > and PHY supports, and decide which MACTYPE setting would be most
> > > suitable. However, to do that we're into also changing phylib's
> > > interfaces as well.
> > >   
> > > > driver to phylink in the call to phylink_create. But there is
> > > > no way for the PHY driver to get this information from phylink
> > > > currently, and even if phylink exposed a function to return the
> > > > config member of struct phylink, the problem is that at the
> > > > time when mv3310_power_up is called, the phydev->phylink is not
> > > > yet set (this is done in phylink_bringup_phy, and
> > > > mv3310_power_up is called sometime in the phylink_attach_phy).
> > > >   
> > > 
> > > We _really_ do not want phylib calling back into phylink
> > > functions. That would tie phylink functionality into phylib and
> > > cause problems when phylink is not being used.
> > > 
> > > I would prefer phylib to be passed "the MAC can use these
> > > interface types, and would prefer to use this interface type" and
> > > have the phylib layer (along with the phylib driver) make the
> > > decision about which mode should be used.  That also means that
> > > non-phylink MACs can also use it.
> > >   
> > 
> > I may try to propose something, but in the meantime do you think the
> > current version of the patch
> >   net: phy: marvell10g: change MACTYPE according to
> > phydev->interface is acceptable?  
> 
> Well, I have other questions about it.  Why are you doing it in
> the power_up function?  Do you find that the MACTYPE field is
> lost when clearing the power down bit?  From what I read, it should
> only change on hardware reset, and we don't hardware reset when we
> come out of power down - only software reset.
> 

Changing MACTYPE requires Port Software Reset. I was not sure if this
wouldn't break something. I am going to try to do this differently.

Marek

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

On Wed, 12 Aug 2020 16:48:37 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 05:37:16PM +0200, Marek Behún wrote:
> > On Wed, 12 Aug 2020 16:00:54 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> >   
> > > On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote:  
> > > > There is another problem though: I think the PHY driver, when
> > > > deciding whether to set MACTYPE from the XFI with rate matching
> > > > mode to the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode,
> > > > should check which modes the underlying MAC support.    
> > > 
> > > I'm aware of that problem.  I have some experimental patches
> > > which add PHY interface mode bitmaps to the MAC, PHY, and SFP
> > > module parsing functions.  I have stumbled on some problems
> > > though - it's going to be another API change (and people are
> > > already whinging about the phylink API changing "too quickly",
> > > were too quickly seems to be defined as once in three years), and
> > > in some cases, DSA, it's extremely hard to work out how to
> > > properly set such a bitmap due to DSA's layered approach.
> > >   
> > 
> > If by your experimental patches you mean
> >   net: mvneta: fill in phy interface mode bitmap
> >   net: mvpp2: fill in phy interface mode bitmap
> > found here
> >   http://git.arm.linux.org.uk/cgit/linux-arm.git/log/?h=clearfog
> > I am currently working on top of them.
> >   
> > > Having bitmaps means that we can take the union of what the MAC
> > > and PHY supports, and decide which MACTYPE setting would be most
> > > suitable. However, to do that we're into also changing phylib's
> > > interfaces as well.
> > >   
> > > > driver to phylink in the call to phylink_create. But there is
> > > > no way for the PHY driver to get this information from phylink
> > > > currently, and even if phylink exposed a function to return the
> > > > config member of struct phylink, the problem is that at the
> > > > time when mv3310_power_up is called, the phydev->phylink is not
> > > > yet set (this is done in phylink_bringup_phy, and
> > > > mv3310_power_up is called sometime in the phylink_attach_phy).
> > > >   
> > > 
> > > We _really_ do not want phylib calling back into phylink
> > > functions. That would tie phylink functionality into phylib and
> > > cause problems when phylink is not being used.
> > > 
> > > I would prefer phylib to be passed "the MAC can use these
> > > interface types, and would prefer to use this interface type" and
> > > have the phylib layer (along with the phylib driver) make the
> > > decision about which mode should be used.  That also means that
> > > non-phylink MACs can also use it.
> > >   
> > 
> > I may try to propose something, but in the meantime do you think the
> > current version of the patch
> >   net: phy: marvell10g: change MACTYPE according to
> > phydev->interface is acceptable?  
> 
> Well, I have other questions about it.  Why are you doing it in
> the power_up function?  Do you find that the MACTYPE field is
> lost when clearing the power down bit?  From what I read, it should
> only change on hardware reset, and we don't hardware reset when we
> come out of power down - only software reset.
> 

The MACTYPE is not being lost. But changing it requires Port Software
Reset, which resets the link, so it cannot be done for example in
read_status.
I think the MACTYPE should be set sometime during PHY initialisation,
and only once: either to XFI with rate matching, if the underlying MAC
does not support lower modes, or to 10gbase-r/2500base-x/sgmii mode, if
the underlying MAC supports only slower modes than 10G.

Marek

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Russell King - ARM Linux admin]

On Wed, Aug 12, 2020 at 06:13:33PM +0200, Marek Behún wrote:
> The MACTYPE is not being lost. But changing it requires Port Software
> Reset, which resets the link, so it cannot be done for example in
> read_status.

Wouldn't the right place to configure it be in the config_init()
method - which is called once we have a MAC attaching to the PHY?
As I mentioned, if we had a way to pass the MAC interface supported
mask into phylib, config_init() could then use that to determine what
to do.

> I think the MACTYPE should be set sometime during PHY initialisation,
> and only once: either to XFI with rate matching, if the underlying MAC
> does not support lower modes, or to 10gbase-r/2500base-x/sgmii mode, if
> the underlying MAC supports only slower modes than 10G.

Yes - only changing the MAC type if we have good reason to do so to
support other rates.

There is a related problem however.  Note that if you have an 88x3310
(non-P) in the SFP, then when rate matching is enabled, the PHY will
_not_ generate pause frames, and the PHY expects the MAC to be
configured to pace itself to the slower speed.  I don't believe we
have support in MACs for that, but phylib and therefore phylink
provides the information:

	interface - 10GBASE-R
	speed - media speed
	pause - media pause modes

So, if speed != SPEED_10000 and there are no pause modes, we should,
for the sake of the entire link, pace the MAC to the media speed by
controlling its egress rate.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

On Wed, 12 Aug 2020 17:22:32 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 06:13:33PM +0200, Marek Behún wrote:
> > The MACTYPE is not being lost. But changing it requires Port
> > Software Reset, which resets the link, so it cannot be done for
> > example in read_status.  
> 
> Wouldn't the right place to configure it be in the config_init()
> method - which is called once we have a MAC attaching to the PHY?
> As I mentioned, if we had a way to pass the MAC interface supported
> mask into phylib, config_init() could then use that to determine what
> to do.
> 

It is done from config_init. mv3310_power_up is called from
mv3310_config_init.

> > I think the MACTYPE should be set sometime during PHY
> > initialisation, and only once: either to XFI with rate matching, if
> > the underlying MAC does not support lower modes, or to
> > 10gbase-r/2500base-x/sgmii mode, if the underlying MAC supports
> > only slower modes than 10G.  
> 
> Yes - only changing the MAC type if we have good reason to do so to
> support other rates.
> 
> There is a related problem however.  Note that if you have an 88x3310
> (non-P) in the SFP, then when rate matching is enabled, the PHY will
> _not_ generate pause frames, and the PHY expects the MAC to be
> configured to pace itself to the slower speed.  I don't believe we
> have support in MACs for that, but phylib and therefore phylink
> provides the information:
> 
> 	interface - 10GBASE-R
> 	speed - media speed
> 	pause - media pause modes
> 
> So, if speed != SPEED_10000 and there are no pause modes, we should,
> for the sake of the entire link, pace the MAC to the media speed by
> controlling its egress rate.
> 

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Russell King - ARM Linux admin]

On Wed, Aug 12, 2020 at 06:28:17PM +0200, Marek Behún wrote:
> On Wed, 12 Aug 2020 17:22:32 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Wed, Aug 12, 2020 at 06:13:33PM +0200, Marek Behún wrote:
> > > The MACTYPE is not being lost. But changing it requires Port
> > > Software Reset, which resets the link, so it cannot be done for
> > > example in read_status.  
> > 
> > Wouldn't the right place to configure it be in the config_init()
> > method - which is called once we have a MAC attaching to the PHY?
> > As I mentioned, if we had a way to pass the MAC interface supported
> > mask into phylib, config_init() could then use that to determine what
> > to do.
> > 
> 
> It is done from config_init. mv3310_power_up is called from
> mv3310_config_init.

I think it'll be best to resume this once we have saner weather.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Russell King - ARM Linux admin]

On Wed, Aug 12, 2020 at 05:37:16PM +0200, Marek Behún wrote:
> On Wed, 12 Aug 2020 16:00:54 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote:
> > > There is another problem though: I think the PHY driver, when
> > > deciding whether to set MACTYPE from the XFI with rate matching
> > > mode to the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode,
> > > should check which modes the underlying MAC support.  
> > 
> > I'm aware of that problem.  I have some experimental patches which add
> > PHY interface mode bitmaps to the MAC, PHY, and SFP module parsing
> > functions.  I have stumbled on some problems though - it's going to be
> > another API change (and people are already whinging about the phylink
> > API changing "too quickly", were too quickly seems to be defined as
> > once in three years), and in some cases, DSA, it's extremely hard to
> > work out how to properly set such a bitmap due to DSA's layered
> > approach.
> > 
> 
> If by your experimental patches you mean
>   net: mvneta: fill in phy interface mode bitmap
>   net: mvpp2: fill in phy interface mode bitmap
> found here
>   http://git.arm.linux.org.uk/cgit/linux-arm.git/log/?h=clearfog
> I am currently working on top of them.

That's fine, because you also have the patches further down in
net-queue.  So, what you list above are basically not all the patches.

net: mvpp2: fill in phy interface mode bitmap
net: mvneta: fill in phy interface mode bitmap
net: phylink: use phy interface mode bitmaps
net: sfp: add interface bitmap
net: phy: add supported_interfaces to marvell10g PHYs
net: phy: add supported_interfaces to marvell PHYs
net: phy: add supported_interfaces to bcm84881
net: phy: add supported_interfaces to phylib

Now, I think there's going to be a problem - if you look at the phylink
code for this, we assume that the supported_interfaces we get from
phylib is what the PHY will be doing.  However, if we then go and change
the interface mode, what then...

I guess if we rework it along the lines that I've mentioned - phylib
is passed the MAC supported interfaces, and phylib/phy drivers choose
which interface to use, then I guess we no longer need to specify to
phylib what interface mode should be used (what do we then do with
the DT phy-mode property?) and we don't need some of the logic in
"net: phylink: use phy interface mode bitmaps".

Please note that it's been a number of months since I last looked at
this, so I may be rusty - it's also rediculously sweltering hot over
here in the UK right now, so I may not be thinking straight... (which
is probably why I forgot to reply to your above point.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

On Wed, 12 Aug 2020 17:01:31 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> That's fine, because you also have the patches further down in
> net-queue.  So, what you list above are basically not all the patches.
> 
> net: mvpp2: fill in phy interface mode bitmap
> net: mvneta: fill in phy interface mode bitmap
> net: phylink: use phy interface mode bitmaps
> net: sfp: add interface bitmap
> net: phy: add supported_interfaces to marvell10g PHYs
> net: phy: add supported_interfaces to marvell PHYs
> net: phy: add supported_interfaces to bcm84881
> net: phy: add supported_interfaces to phylib

I did not list them all, but I applied them almost all your patches
from clearfog branch to linus' master and am working on top of that

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Andrew Lunn]

> I'm aware of that problem.  I have some experimental patches which add
> PHY interface mode bitmaps to the MAC, PHY, and SFP module parsing
> functions.  I have stumbled on some problems though - it's going to be
> another API change (and people are already whinging about the phylink
> API changing "too quickly", were too quickly seems to be defined as
> once in three years), and in some cases, DSA, it's extremely hard to
> work out how to properly set such a bitmap due to DSA's layered
> approach.

Hi Russell

If DSAs layering is causing real problems, we could rip it out, and
let the driver directly interact with phylink. I'm not opposed to
that.

	Andrew

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Russell King - ARM Linux admin]

On Wed, Aug 12, 2020 at 05:44:36PM +0200, Andrew Lunn wrote:
> > I'm aware of that problem.  I have some experimental patches which add
> > PHY interface mode bitmaps to the MAC, PHY, and SFP module parsing
> > functions.  I have stumbled on some problems though - it's going to be
> > another API change (and people are already whinging about the phylink
> > API changing "too quickly", were too quickly seems to be defined as
> > once in three years), and in some cases, DSA, it's extremely hard to
> > work out how to properly set such a bitmap due to DSA's layered
> > approach.
> 
> Hi Russell
> 
> If DSAs layering is causing real problems, we could rip it out, and
> let the driver directly interact with phylink. I'm not opposed to
> that.

The reason I mentioned it is because I have this unpublished patch
(beware, whitespace damaged due to copy-n-pasted):

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index c1967e08b017..ba32492f3ec0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1629,6 +1629,12 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)

        dp->pl_config.dev = &slave_dev->dev;
        dp->pl_config.type = PHYLINK_NETDEV;
+       __set_bit(PHY_INTERFACE_MODE_SGMII,
+                 dp->pl_config.supported_interfaces);
+       __set_bit(PHY_INTERFACE_MODE_2500BASEX,
+                 dp->pl_config.supported_interfaces);
+       __set_bit(PHY_INTERFACE_MODE_1000BASEX,
+                 dp->pl_config.supported_interfaces);

        /* The get_fixed_state callback takes precedence over polling the
         * link GPIO in PHYLINK (see phylink_get_fixed_state).  Only set

Which clearly is a gross hack - this code certainly has no idea about
what interfaces the port itself supports.  How do we get around that
with DSA layering?

We could add yet-another-driver-call down into the DSA driver for it
to fill in that information and keep the current structure.  However,
is that really the best solution - to have lots of fine grained driver
calls?

DSA feels to be very cumbersome and awkward to modify at least to me.
Almost everything seems to be "add another driver call" at the DSA
layer, followed by "add another sub-driver call" at the mv88e6xxx
layer.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

[Marek Behún]

On Wed, 12 Aug 2020 16:54:41 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 05:44:36PM +0200, Andrew Lunn wrote:
> > > I'm aware of that problem.  I have some experimental patches
> > > which add PHY interface mode bitmaps to the MAC, PHY, and SFP
> > > module parsing functions.  I have stumbled on some problems
> > > though - it's going to be another API change (and people are
> > > already whinging about the phylink API changing "too quickly",
> > > were too quickly seems to be defined as once in three years), and
> > > in some cases, DSA, it's extremely hard to work out how to
> > > properly set such a bitmap due to DSA's layered approach.  
> > 
> > Hi Russell
> > 
> > If DSAs layering is causing real problems, we could rip it out, and
> > let the driver directly interact with phylink. I'm not opposed to
> > that.  
> 
> The reason I mentioned it is because I have this unpublished patch
> (beware, whitespace damaged due to copy-n-pasted):
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index c1967e08b017..ba32492f3ec0 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1629,6 +1629,12 @@ static int dsa_slave_phy_setup(struct
> net_device *slave_dev)
> 
>         dp->pl_config.dev = &slave_dev->dev;
>         dp->pl_config.type = PHYLINK_NETDEV;
> +       __set_bit(PHY_INTERFACE_MODE_SGMII,
> +                 dp->pl_config.supported_interfaces);
> +       __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +                 dp->pl_config.supported_interfaces);
> +       __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +                 dp->pl_config.supported_interfaces);
> 
>         /* The get_fixed_state callback takes precedence over polling
> the
>          * link GPIO in PHYLINK (see phylink_get_fixed_state).  Only
> set
> 
> Which clearly is a gross hack - this code certainly has no idea about
> what interfaces the port itself supports.  How do we get around that
> with DSA layering?
> 
> We could add yet-another-driver-call down into the DSA driver for it
> to fill in that information and keep the current structure.  However,
> is that really the best solution - to have lots of fine grained driver
> calls?
> 
> DSA feels to be very cumbersome and awkward to modify at least to me.
> Almost everything seems to be "add another driver call" at the DSA
> layer, followed by "add another sub-driver call" at the mv88e6xxx
> layer.
> 

So I am encountering this now when testing my SFP + marvell10g patches
on a board with SFP cage on a DSA port.

I get what you find cumbersome, but I don't think there is other
reasonable solution here. We already have .phylink_validate, which
works with ethtool mode mask. So maybe a .phylink_config_init ?

Marek

[PATCH RFC russell-king 4/4] net: phylink: don't fail attaching phy on 1000base-x/2500base-x mode

[Marek Behún]

Some SFPs may contain an internal PHY which may in some cases want to
connect with the host interface in 1000base-x/2500base-x mode.
Do not fail if such PHY is being attached in one of these PHY interface
modes.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/phylink.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 83e14176baba1..e20f6770b5c4b 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1015,9 +1015,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 			      phy_interface_t interface)
 {
-	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
-		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
-		     phy_interface_mode_is_8023z(interface))))
+	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED))
 		return -EINVAL;
 
 	if (pl->phydev)
-- 
2.26.2

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Russell King - ARM Linux admin]

On Tue, Aug 11, 2020 at 12:06:41AM +0200, Marek Behún wrote:
> Hi Russell,
> 
> this series should apply on linux-arm git repository, on branch
> clearfog.
> 
> Some internet providers are already starting to offer 2.5G copper
> connectivity to their users. On Turris Omnia the SFP port is capable
> of 2.5G speed, so we tested some copper SFP modules.
> 
> This adds support to the SFP subsystem for 10G RollBall copper modules
> which contain a Marvell 88X3310 PHY. By default these modules are
> configured in 10GKR only mode on the host interface, and also contain
> some bad information in EEPROM (the extended_cc byte).

Are you sure they are 10GBASE-KR, and not 10GBASE-R ?  Please send me
the contents of MMD 31 register 0xf001.  Also knowing MMD 1 registers
2 and 3 would be useful to confirm exactly which version of the PHY
has been used.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

On Tue, 11 Aug 2020 16:08:09 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> Are you sure they are 10GBASE-KR, and not 10GBASE-R ?  Please send me
> the contents of MMD 31 register 0xf001.  Also knowing MMD 1 registers
> 2 and 3 would be useful to confirm exactly which version of the PHY
> has been used.
> 
> Thanks.
> 

1f:f001 = 0x0004
01:0002 = 0x002b
01:0003 = 0x09ab

Marek

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

On Wed, 12 Aug 2020 14:31:03 +0200
Marek Behún <marek.behun@nic.cz> wrote:

> On Tue, 11 Aug 2020 16:08:09 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > Are you sure they are 10GBASE-KR, and not 10GBASE-R ?  Please send
> > me the contents of MMD 31 register 0xf001.  Also knowing MMD 1
> > registers 2 and 3 would be useful to confirm exactly which version
> > of the PHY has been used.
> > 
> > Thanks.
> >   
> 
> 1f:f001 = 0x0004
> 01:0002 = 0x002b
> 01:0003 = 0x09ab
> 
> Marek

Sorry, 1f:f001 is changed to 0x0004 by my change to the marvell10g
driver. By default it is configured to 0x0006 (XFI with rate matching)

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

On Tue, 11 Aug 2020 16:08:09 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Tue, Aug 11, 2020 at 12:06:41AM +0200, Marek Behún wrote:
> > Hi Russell,
> > 
> > this series should apply on linux-arm git repository, on branch
> > clearfog.
> > 
> > Some internet providers are already starting to offer 2.5G copper
> > connectivity to their users. On Turris Omnia the SFP port is capable
> > of 2.5G speed, so we tested some copper SFP modules.
> > 
> > This adds support to the SFP subsystem for 10G RollBall copper
> > modules which contain a Marvell 88X3310 PHY. By default these
> > modules are configured in 10GKR only mode on the host interface,
> > and also contain some bad information in EEPROM (the extended_cc
> > byte).  
> 
> Are you sure they are 10GBASE-KR, and not 10GBASE-R ?  Please send me
> the contents of MMD 31 register 0xf001.  Also knowing MMD 1 registers
> 2 and 3 would be useful to confirm exactly which version of the PHY
> has been used.
> 
> Thanks.
> 

Russell, sorry, I meant 10gbase-r mode, this was a typo. I don't know if
the PHY supports 10gbase-kr.

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Russell King - ARM Linux admin]

On Tue, Aug 11, 2020 at 12:06:41AM +0200, Marek Behún wrote:
> Hi Russell,
> 
> this series should apply on linux-arm git repository, on branch
> clearfog.

How about something like this - only build tested, and you may
encounter fuzz with this:

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 147b4cf4188e..bcbef68e0917 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -117,6 +117,7 @@ enum {
 	MV_V2_PORT_CTRL_FT_1000BASEX = 0 << 3,
 	MV_V2_PORT_CTRL_FT_SGMII = 1 << 3,
 	MV_V2_PORT_CTRL_FT_10GBASER = 3 << 3,
+	MV_V2_PORT_CTRL_MACTYPE	= 7 << 0,
 	MV_V2_UIS		= 0xf040,
 	MV_V2_PIS		= 0xf042,
 	MV_V2_PIS_PI		= BIT(0),
@@ -691,17 +692,44 @@ static bool mv3310_has_pma_ngbaset_quirk(struct phy_device *phydev)
 		MV_PHY_ALASKA_NBT_QUIRK_MASK) == MV_PHY_ALASKA_NBT_QUIRK_REV;
 }
 
+static int mv3310_select_mode(struct phy_device *phydev,
+			      unsigned long *host_interfaces)
+{
+	int mac_type = -1;
+
+	if (test_bit(PHY_INTERFACE_MODE_USXGMII, host_interfaces))
+		mac_type = 7;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_10GBASER, host_interfaces))
+		mac_type = 4;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_RXAUI, host_interfaces))
+		mac_type = 0;
+	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, host_interfaces))
+		mac_type = 6;
+	else if (test_bit(PHY_INTERFACE_MODE_RXAUI, host_interfaces))
+		mac_type = 2;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces))
+		mac_type = 4;
+
+	return mac_type;
+}
+
 static int mv3310_config_init(struct phy_device *phydev)
 {
-	int err;
+	int ret, err, mac_type = -1;
 
 	/* Check that the PHY interface type is compatible */
-	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
-	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
-	    phydev->interface != PHY_INTERFACE_MODE_XAUI &&
-	    phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
-	    phydev->interface != PHY_INTERFACE_MODE_10GBASER)
+	if (!phy_interface_empty(phydev->host_interfaces)) {
+		mac_type = mv3310_select_mode(phydev, phydev->host_interfaces);
+		phydev_info(phydev, "mac_type=%d\n", mac_type);
+	} else if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+		   phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
+		   phydev->interface != PHY_INTERFACE_MODE_XAUI &&
+		   phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
+		   phydev->interface != PHY_INTERFACE_MODE_10GBASER) {
 		return -ENODEV;
+	}
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 
@@ -710,6 +738,20 @@ static int mv3310_config_init(struct phy_device *phydev)
 	if (err)
 		return err;
 
+	if (mac_type != -1) {
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND2,
+					     MV_V2_PORT_CTRL,
+					     MV_V2_PORT_CTRL_MACTYPE, mac_type);
+		if (ret > 0)
+			ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+					     MV_V2_PORT_CTRL,
+					     MV_V2_PORT_CTRL_SWRST,
+					     MV_V2_PORT_CTRL_SWRST);
+
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Enable EDPD mode - saving 600mW */
 	err = mv3310_set_edpd(phydev, ETHTOOL_PHY_EDPD_DFLT_TX_MSECS);
 	if (err)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5785eb040f11..4ad64973432a 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2082,6 +2082,8 @@ static void phylink_sfp_detach(void *upstream, struct sfp_bus *bus)
 	sfp_bus_unlink_netdev(bus, pl->netdev);
 }
 
+static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces);
+
 static const phy_interface_t phylink_sfp_interface_preference[] = {
 	PHY_INTERFACE_MODE_USXGMII,
 	PHY_INTERFACE_MODE_10GBASER,
@@ -2091,6 +2093,18 @@ static const phy_interface_t phylink_sfp_interface_preference[] = {
 	PHY_INTERFACE_MODE_1000BASEX,
 };
 
+static int __init phylink_init(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++)
+		set_bit(phylink_sfp_interface_preference[i],
+			phylink_sfp_interfaces);
+
+	return 0;
+}
+module_init(phylink_init);
+
 static phy_interface_t phylink_select_interface(struct phylink *pl,
 						const unsigned long *intf,
 						const char *intf_name)
@@ -2342,6 +2356,10 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	else
 		mode = MLO_AN_INBAND;
 
+	/* Set the PHY's host supported interfaces */
+	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
+			  pl->config->supported_interfaces);
+
 	if (!phy_interface_empty(phy->supported_interfaces) &&
 	    !phy_interface_empty(pl->config->supported_interfaces)) {
 		interface = phylink_select_interface(pl,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7408e2240c1e..14f73378f4e9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -527,6 +527,7 @@ struct phy_device {
 
 	/* bitmap of supported interfaces */
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
+	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
 
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

On Mon, 17 Aug 2020 14:49:09 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Tue, Aug 11, 2020 at 12:06:41AM +0200, Marek Behún wrote:
> > Hi Russell,
> > 
> > this series should apply on linux-arm git repository, on branch
> > clearfog.  
> 
> How about something like this - only build tested, and you may
> encounter fuzz with this:
> 
> diff --git a/drivers/net/phy/marvell10g.c
> b/drivers/net/phy/marvell10g.c index 147b4cf4188e..bcbef68e0917 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -117,6 +117,7 @@ enum {
>  	MV_V2_PORT_CTRL_FT_1000BASEX = 0 << 3,
>  	MV_V2_PORT_CTRL_FT_SGMII = 1 << 3,
>  	MV_V2_PORT_CTRL_FT_10GBASER = 3 << 3,
> +	MV_V2_PORT_CTRL_MACTYPE	= 7 << 0,
>  	MV_V2_UIS		= 0xf040,
>  	MV_V2_PIS		= 0xf042,
>  	MV_V2_PIS_PI		= BIT(0),
> @@ -691,17 +692,44 @@ static bool mv3310_has_pma_ngbaset_quirk(struct
> phy_device *phydev) MV_PHY_ALASKA_NBT_QUIRK_MASK) ==
> MV_PHY_ALASKA_NBT_QUIRK_REV; }
>  
> +static int mv3310_select_mode(struct phy_device *phydev,
> +			      unsigned long *host_interfaces)
> +{
> +	int mac_type = -1;
> +
> +	if (test_bit(PHY_INTERFACE_MODE_USXGMII, host_interfaces))
> +		mac_type = 7;
> +	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces)
> &&
> +		 test_bit(PHY_INTERFACE_MODE_10GBASER,
> host_interfaces))
> +		mac_type = 4;
> +	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces)
> &&
> +		 test_bit(PHY_INTERFACE_MODE_RXAUI, host_interfaces))
> +		mac_type = 0;
> +	else if (test_bit(PHY_INTERFACE_MODE_10GBASER,
> host_interfaces))
> +		mac_type = 6;
> +	else if (test_bit(PHY_INTERFACE_MODE_RXAUI, host_interfaces))
> +		mac_type = 2;
> +	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces))
> +		mac_type = 4;
> +
> +	return mac_type;
> +}
> +
>  static int mv3310_config_init(struct phy_device *phydev)
>  {
> -	int err;
> +	int ret, err, mac_type = -1;
>  
>  	/* Check that the PHY interface type is compatible */
> -	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
> -	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
> -	    phydev->interface != PHY_INTERFACE_MODE_XAUI &&
> -	    phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
> -	    phydev->interface != PHY_INTERFACE_MODE_10GBASER)
> +	if (!phy_interface_empty(phydev->host_interfaces)) {
> +		mac_type = mv3310_select_mode(phydev,
> phydev->host_interfaces);
> +		phydev_info(phydev, "mac_type=%d\n", mac_type);
> +	} else if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
> +		   phydev->interface != PHY_INTERFACE_MODE_2500BASEX
> &&
> +		   phydev->interface != PHY_INTERFACE_MODE_XAUI &&
> +		   phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
> +		   phydev->interface != PHY_INTERFACE_MODE_10GBASER)
> { return -ENODEV;
> +	}
>  
>  	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>  
> @@ -710,6 +738,20 @@ static int mv3310_config_init(struct phy_device
> *phydev) if (err)
>  		return err;
>  
> +	if (mac_type != -1) {
> +		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND2,
> +					     MV_V2_PORT_CTRL,
> +
> MV_V2_PORT_CTRL_MACTYPE, mac_type);
> +		if (ret > 0)
> +			ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +					     MV_V2_PORT_CTRL,
> +					     MV_V2_PORT_CTRL_SWRST,
> +					     MV_V2_PORT_CTRL_SWRST);

When chaning mactype you also have to issue SWRST in the same register
write. Otherwise it did not work for me.

> +
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	/* Enable EDPD mode - saving 600mW */
>  	err = mv3310_set_edpd(phydev,
> ETHTOOL_PHY_EDPD_DFLT_TX_MSECS); if (err)
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 5785eb040f11..4ad64973432a 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2082,6 +2082,8 @@ static void phylink_sfp_detach(void *upstream,
> struct sfp_bus *bus) sfp_bus_unlink_netdev(bus, pl->netdev);
>  }
>  
> +static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces);
> +
>  static const phy_interface_t phylink_sfp_interface_preference[] = {
>  	PHY_INTERFACE_MODE_USXGMII,
>  	PHY_INTERFACE_MODE_10GBASER,
> @@ -2091,6 +2093,18 @@ static const phy_interface_t
> phylink_sfp_interface_preference[] = { PHY_INTERFACE_MODE_1000BASEX,
>  };
>  
> +static int __init phylink_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i <
> ARRAY_SIZE(phylink_sfp_interface_preference); i++)
> +		set_bit(phylink_sfp_interface_preference[i],
> +			phylink_sfp_interfaces);
> +
> +	return 0;
> +}
> +module_init(phylink_init);
> +
>  static phy_interface_t phylink_select_interface(struct phylink *pl,
>  						const unsigned long
> *intf, const char *intf_name)
> @@ -2342,6 +2356,10 @@ static int phylink_sfp_connect_phy(void
> *upstream, struct phy_device *phy) else
>  		mode = MLO_AN_INBAND;
>  
> +	/* Set the PHY's host supported interfaces */
> +	phy_interface_and(phy->host_interfaces,
> phylink_sfp_interfaces,
> +			  pl->config->supported_interfaces);
> +
>  	if (!phy_interface_empty(phy->supported_interfaces) &&
>  	    !phy_interface_empty(pl->config->supported_interfaces)) {
>  		interface = phylink_select_interface(pl,
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 7408e2240c1e..14f73378f4e9 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -527,6 +527,7 @@ struct phy_device {
>  
>  	/* bitmap of supported interfaces */
>  	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
> +	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
>  
>  	/* Energy efficient ethernet modes which should be
> prohibited */ u32 eee_broken_modes;
> 

Otherwise it looks nice. I will test this. On what branch does this
apply?

Marek

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Russell King - ARM Linux admin]

On Tue, Aug 18, 2020 at 03:43:05PM +0200, Marek Behún wrote:
> On Mon, 17 Aug 2020 14:49:09 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > @@ -710,6 +738,20 @@ static int mv3310_config_init(struct phy_device
> > *phydev) if (err)
> >  		return err;
> >  
> > +	if (mac_type != -1) {
> > +		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND2,
> > +					     MV_V2_PORT_CTRL,
> > +
> > MV_V2_PORT_CTRL_MACTYPE, mac_type);
> > +		if (ret > 0)
> > +			ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > +					     MV_V2_PORT_CTRL,
> > +					     MV_V2_PORT_CTRL_SWRST,
> > +					     MV_V2_PORT_CTRL_SWRST);
> 
> When chaning mactype you also have to issue SWRST in the same register
> write. Otherwise it did not work for me.

That's interesting - because the documentation explicitly states that
this is a two-step process.

> Otherwise it looks nice. I will test this. On what branch does this
> apply?

My unpublished tip of the universe development.  Here's a version on
the clearfog branch:

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index fe7e30c3faa8..dd282844637b 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -95,6 +95,7 @@ enum {
 	MV_V2_PORT_CTRL		= 0xf001,
 	MV_V2_PORT_CTRL_SWRST	= BIT(15),
 	MV_V2_PORT_CTRL_PWRDOWN = BIT(11),
+	MV_V2_PORT_CTRL_MACTYPE	= 7 << 0,
 	/* Temperature control/read registers (88X3310 only) */
 	MV_V2_TEMP_CTRL		= 0xf08a,
 	MV_V2_TEMP_CTRL_MASK	= 0xc000,
@@ -597,17 +598,44 @@ static bool mv3310_has_pma_ngbaset_quirk(struct phy_device *phydev)
 		MV_PHY_ALASKA_NBT_QUIRK_MASK) == MV_PHY_ALASKA_NBT_QUIRK_REV;
 }
 
+static int mv3310_select_mode(struct phy_device *phydev,
+			      unsigned long *host_interfaces)
+{
+	int mac_type = -1;
+
+	if (test_bit(PHY_INTERFACE_MODE_USXGMII, host_interfaces))
+		mac_type = 7;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_10GBASER, host_interfaces))
+		mac_type = 4;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_RXAUI, host_interfaces))
+		mac_type = 0;
+	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, host_interfaces))
+		mac_type = 6;
+	else if (test_bit(PHY_INTERFACE_MODE_RXAUI, host_interfaces))
+		mac_type = 2;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces))
+		mac_type = 4;
+
+	return mac_type;
+}
+
 static int mv3310_config_init(struct phy_device *phydev)
 {
-	int err;
+	int ret, err, mac_type = -1;
 
 	/* Check that the PHY interface type is compatible */
-	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
-	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
-	    phydev->interface != PHY_INTERFACE_MODE_XAUI &&
-	    phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
-	    phydev->interface != PHY_INTERFACE_MODE_10GBASER)
+	if (!phy_interface_empty(phydev->host_interfaces)) {
+		mac_type = mv3310_select_mode(phydev, phydev->host_interfaces);
+		phydev_info(phydev, "mac_type=%d\n", mac_type);
+	} else if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+		   phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
+		   phydev->interface != PHY_INTERFACE_MODE_XAUI &&
+		   phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
+		   phydev->interface != PHY_INTERFACE_MODE_10GBASER) {
 		return -ENODEV;
+	}
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 
@@ -616,6 +644,20 @@ static int mv3310_config_init(struct phy_device *phydev)
 	if (err)
 		return err;
 
+	if (mac_type != -1) {
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND2,
+					     MV_V2_PORT_CTRL,
+					     MV_V2_PORT_CTRL_MACTYPE, mac_type);
+		if (ret > 0)
+			ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+					     MV_V2_PORT_CTRL,
+					     MV_V2_PORT_CTRL_SWRST,
+					     MV_V2_PORT_CTRL_SWRST);
+
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Enable EDPD mode - saving 600mW */
 	err = mv3310_set_edpd(phydev, ETHTOOL_PHY_EDPD_DFLT_TX_MSECS);
 	if (err)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index ac44fceb84e6..ac9b1a4fe464 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2016,6 +2016,8 @@ static void phylink_sfp_detach(void *upstream, struct sfp_bus *bus)
 	pl->netdev->sfp_bus = NULL;
 }
 
+static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces);
+
 static const phy_interface_t phylink_sfp_interface_preference[] = {
 	PHY_INTERFACE_MODE_USXGMII,
 	PHY_INTERFACE_MODE_10GBASER,
@@ -2025,6 +2027,18 @@ static const phy_interface_t phylink_sfp_interface_preference[] = {
 	PHY_INTERFACE_MODE_1000BASEX,
 };
 
+static int __init phylink_init(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++)
+		set_bit(phylink_sfp_interface_preference[i],
+			phylink_sfp_interfaces);
+
+	return 0;
+}
+module_init(phylink_init);
+
 static phy_interface_t phylink_select_interface(struct phylink *pl,
 						const unsigned long *intf,
 						const char *intf_name)
@@ -2239,6 +2253,10 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	else
 		mode = MLO_AN_INBAND;
 
+	/* Set the PHY's host supported interfaces */
+	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
+			  pl->config->supported_interfaces);
+
 	if (!phy_interface_empty(phy->supported_interfaces) &&
 	    !phy_interface_empty(pl->config->supported_interfaces)) {
 		interface = phylink_select_interface(pl,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index b7a51f09dad7..9ce4b6c5ebef 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -527,6 +527,7 @@ struct phy_device {
 
 	/* bitmap of supported interfaces */
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
+	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
 
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

On Tue, 18 Aug 2020 16:08:35 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> > Otherwise it looks nice. I will test this. On what branch does this
> > apply?  
> 
> My unpublished tip of the universe development.  Here's a version on
> the clearfog branch:

Russell, it seems you do not have commit

e11703330a5d ("net: phy: marvell10g: support XFI rate matching mode")

in that branch.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/marvell10g.c?id=e11703330a5df48e1fd4167e4d22a102e517253e

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Russell King - ARM Linux admin]

On Tue, Aug 18, 2020 at 05:30:55PM +0200, Marek Behún wrote:
> On Tue, 18 Aug 2020 16:08:35 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > > Otherwise it looks nice. I will test this. On what branch does this
> > > apply?  
> > 
> > My unpublished tip of the universe development.  Here's a version on
> > the clearfog branch:
> 
> Russell, it seems you do not have commit
> 
> e11703330a5d ("net: phy: marvell10g: support XFI rate matching mode")
> 
> in that branch.

I don't.  That commit is in 5.9-rc1, I'm based on 5.8 at the moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

On Tue, 18 Aug 2020 16:36:49 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Tue, Aug 18, 2020 at 05:30:55PM +0200, Marek Behún wrote:
> > On Tue, 18 Aug 2020 16:08:35 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> >   
> > > > Otherwise it looks nice. I will test this. On what branch does
> > > > this apply?    
> > > 
> > > My unpublished tip of the universe development.  Here's a version
> > > on the clearfog branch:  
> > 
> > Russell, it seems you do not have commit
> > 
> > e11703330a5d ("net: phy: marvell10g: support XFI rate matching
> > mode")
> > 
> > in that branch.  
> 
> I don't.  That commit is in 5.9-rc1, I'm based on 5.8 at the moment.
> 

I am reworking it so that it applies on top of master together with
your other commits from clearfog branch.

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Russell King - ARM Linux admin]

On Tue, Aug 18, 2020 at 05:47:24PM +0200, Marek Behún wrote:
> On Tue, 18 Aug 2020 16:36:49 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Tue, Aug 18, 2020 at 05:30:55PM +0200, Marek Behún wrote:
> > > On Tue, 18 Aug 2020 16:08:35 +0100
> > > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > >   
> > > > > Otherwise it looks nice. I will test this. On what branch does
> > > > > this apply?    
> > > > 
> > > > My unpublished tip of the universe development.  Here's a version
> > > > on the clearfog branch:  
> > > 
> > > Russell, it seems you do not have commit
> > > 
> > > e11703330a5d ("net: phy: marvell10g: support XFI rate matching
> > > mode")
> > > 
> > > in that branch.  
> > 
> > I don't.  That commit is in 5.9-rc1, I'm based on 5.8 at the moment.
> > 
> 
> I am reworking it so that it applies on top of master together with
> your other commits from clearfog branch.

If it works for you, I'll rework my branches to adopt this approach.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

On Tue, 18 Aug 2020 17:34:15 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Tue, Aug 18, 2020 at 05:47:24PM +0200, Marek Behún wrote:
> > On Tue, 18 Aug 2020 16:36:49 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> >   
> > > On Tue, Aug 18, 2020 at 05:30:55PM +0200, Marek Behún wrote:  
> > > > On Tue, 18 Aug 2020 16:08:35 +0100
> > > > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > >     
> > > > > > Otherwise it looks nice. I will test this. On what branch
> > > > > > does this apply?      
> > > > > 
> > > > > My unpublished tip of the universe development.  Here's a
> > > > > version on the clearfog branch:    
> > > > 
> > > > Russell, it seems you do not have commit
> > > > 
> > > > e11703330a5d ("net: phy: marvell10g: support XFI rate matching
> > > > mode")
> > > > 
> > > > in that branch.    
> > > 
> > > I don't.  That commit is in 5.9-rc1, I'm based on 5.8 at the
> > > moment. 
> > 
> > I am reworking it so that it applies on top of master together with
> > your other commits from clearfog branch.  
> 
> If it works for you, I'll rework my branches to adopt this approach.
> 

Russell,

if you have time please rebase your work on top of Linus' master or
net-next.

Btw one of your patches in clearfog branch breaks mvneta
(although there are other patches which fix this break).
Check this out:
  patch
      net: mvneta: move 1ms clock control into mac_prepare/mac_finish
  removes definition of variables new_clk and gmac_clk in
  mvneta_mac_config, but does not remove one instance of usage, which
  can be seen in subsequent patch
      net: mvneta: convert to phylink pcs operations
  this is fixed by patch
      net: mvneta: split out GMAC
  where all of this is moved to mvgmac.c file.
I found out because I did not apply the last patch for some reason
(maybe it didn't apply on Linus' master or something, I don't remember),
and it failed to compile.

http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=clearfog&id=ca096c11e6798b1f4da8466ab0c3bf42b6e9fb81
http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=clearfog&id=1e345c538cc73d0b0a63c01a13e69a865905b7ae
http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=clearfog&id=dc33cc833537d06b198c9226d4a230cfc33569a6

I have just sent patches that add 88E6393X switch to mv88e6xxx driver,
since I am testing these SFPs on a Marvell Customer Reference Board
containing this switch.

The board also contains the other PHY supported by the marvell10g PHY
driver, 88E2110, so I will test my changes also on this PHY.

I am waiting for newer documentation for 88E3110, since the one I have
is outdated and does not contain descriptions on how to resolve USXGMII
auto-negotiation, which I would also like to add.

Marek

Re: [PATCH RFC russell-king 0/4] Support for RollBall 10G copper SFP modules

[Marek Behún]

Btw if you want to check out the current status of my work, you can find
it at

https://gitlab.nic.cz/turris/mox-kernel/-/tree/sfp-2020-08-work

the last patch is the one for marvell10g

On Wed, 19 Aug 2020 17:49:50 +0200
Marek Behún <marek.behun@nic.cz> wrote:

> Russell,
> 
> if you have time please rebase your work on top of Linus' master or
> net-next.
> 
> Btw one of your patches in clearfog branch breaks mvneta
> (although there are other patches which fix this break).
> Check this out:
>   patch
>       net: mvneta: move 1ms clock control into mac_prepare/mac_finish
>   removes definition of variables new_clk and gmac_clk in
>   mvneta_mac_config, but does not remove one instance of usage, which
>   can be seen in subsequent patch
>       net: mvneta: convert to phylink pcs operations
>   this is fixed by patch
>       net: mvneta: split out GMAC
>   where all of this is moved to mvgmac.c file.
> I found out because I did not apply the last patch for some reason
> (maybe it didn't apply on Linus' master or something, I don't
> remember), and it failed to compile.
> 
> http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=clearfog&id=ca096c11e6798b1f4da8466ab0c3bf42b6e9fb81
> http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=clearfog&id=1e345c538cc73d0b0a63c01a13e69a865905b7ae
> http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=clearfog&id=dc33cc833537d06b198c9226d4a230cfc33569a6
> 
> I have just sent patches that add 88E6393X switch to mv88e6xxx driver,
> since I am testing these SFPs on a Marvell Customer Reference Board
> containing this switch.
> 
> The board also contains the other PHY supported by the marvell10g PHY
> driver, 88E2110, so I will test my changes also on this PHY.
> 
> I am waiting for newer documentation for 88E3110, since the one I have
> is outdated and does not contain descriptions on how to resolve
> USXGMII auto-negotiation, which I would also like to add.
> 
> Marek