[PATCH net-next] net: sfp: Set 1000BaseX support flag for 1000BaseT modules

[PATCH net-next] net: sfp: Set 1000BaseX support flag for 1000BaseT modules

[Robert Hancock]

Modules which support 1000BaseT should also have 1000BaseX support. Set
this support flag to allow drivers supporting only 1000BaseX to work.

Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
---
 drivers/net/phy/sfp-bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index e9c1879..96cdf85 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -158,6 +158,7 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 	if (id->base.e1000_base_t) {
 		phylink_set(modes, 1000baseT_Half);
 		phylink_set(modes, 1000baseT_Full);
+		phylink_set(modes, 1000baseX_Full);
 	}
 
 	/* 1000Base-PX or 1000Base-BX10 */
-- 
1.8.3.1

[PATCH net-next] net: sfp: Stop SFP polling during shutdown

[Robert Hancock]

SFP device polling can cause problems during the shutdown process if the
parent devices of the network controller have been shut down already.
This problem was seen on the iMX6 platform with PCIe devices, where
accessing the device after the bus is shut down causes a hang.

Stop all delayed work in the SFP driver during the shutdown process to
avoid this problem.

Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
---
 drivers/net/phy/sfp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 554acc8..6b6c83d 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1928,9 +1928,18 @@ static int sfp_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void sfp_shutdown(struct platform_device *pdev)
+{
+	struct sfp *sfp = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&sfp->poll);
+	cancel_delayed_work_sync(&sfp->timeout);
+}
+
 static struct platform_driver sfp_driver = {
 	.probe = sfp_probe,
 	.remove = sfp_remove,
+	.shutdown = sfp_shutdown,
 	.driver = {
 		.name = "sfp",
 		.of_match_table = sfp_of_match,
-- 
1.8.3.1

Re: [PATCH net-next] net: sfp: Stop SFP polling during shutdown

[Russell King - ARM Linux admin]

On Fri, May 31, 2019 at 01:18:02PM -0600, Robert Hancock wrote:
> SFP device polling can cause problems during the shutdown process if the
> parent devices of the network controller have been shut down already.
> This problem was seen on the iMX6 platform with PCIe devices, where
> accessing the device after the bus is shut down causes a hang.
> 
> Stop all delayed work in the SFP driver during the shutdown process to
> avoid this problem.

What about interrupts?

> 
> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> ---
>  drivers/net/phy/sfp.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 554acc8..6b6c83d 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -1928,9 +1928,18 @@ static int sfp_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void sfp_shutdown(struct platform_device *pdev)
> +{
> +	struct sfp *sfp = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&sfp->poll);
> +	cancel_delayed_work_sync(&sfp->timeout);
> +}
> +
>  static struct platform_driver sfp_driver = {
>  	.probe = sfp_probe,
>  	.remove = sfp_remove,
> +	.shutdown = sfp_shutdown,
>  	.driver = {
>  		.name = "sfp",
>  		.of_match_table = sfp_of_match,
> -- 
> 1.8.3.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next] net: sfp: Stop SFP polling during shutdown

[Robert Hancock]

On 2019-05-31 2:12 p.m., Russell King - ARM Linux admin wrote:
> On Fri, May 31, 2019 at 01:18:02PM -0600, Robert Hancock wrote:
>> SFP device polling can cause problems during the shutdown process if the
>> parent devices of the network controller have been shut down already.
>> This problem was seen on the iMX6 platform with PCIe devices, where
>> accessing the device after the bus is shut down causes a hang.
>>
>> Stop all delayed work in the SFP driver during the shutdown process to
>> avoid this problem.
> 
> What about interrupts?

Indeed, missed that as the GPIO controller this was being used with
didn't support interrupts. Will update the patch.

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

[PATCH net-next] net: sfp: Use smaller chunk size when reading I2C data

[Robert Hancock]

The SFP driver was reading up to 256 bytes of I2C data from the SFP
module in a single chunk. However, some I2C controllers do not support
reading that many bytes in a single transaction. Change to use a more
compatible 16-byte chunk size, since this is not performance critical.

Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
---
 drivers/net/phy/sfp.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 6b6c83d..23a40a7 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1651,7 +1651,7 @@ static int sfp_module_info(struct sfp *sfp, struct ethtool_modinfo *modinfo)
 static int sfp_module_eeprom(struct sfp *sfp, struct ethtool_eeprom *ee,
 			     u8 *data)
 {
-	unsigned int first, last, len;
+	unsigned int first, last;
 	int ret;
 
 	if (ee->len == 0)
@@ -1659,26 +1659,36 @@ static int sfp_module_eeprom(struct sfp *sfp, struct ethtool_eeprom *ee,
 
 	first = ee->offset;
 	last = ee->offset + ee->len;
-	if (first < ETH_MODULE_SFF_8079_LEN) {
-		len = min_t(unsigned int, last, ETH_MODULE_SFF_8079_LEN);
-		len -= first;
 
-		ret = sfp_read(sfp, false, first, data, len);
+	while (first < last) {
+		bool a2;
+		unsigned int this_addr, len;
+
+		if (first < ETH_MODULE_SFF_8079_LEN) {
+			len = min_t(unsigned int, last,
+				    ETH_MODULE_SFF_8079_LEN);
+			len -= first;
+			a2 = false;
+			this_addr = first;
+		} else {
+			len = min_t(unsigned int, last,
+				    ETH_MODULE_SFF_8472_LEN);
+			len -= first;
+			a2 = true;
+			this_addr = first - ETH_MODULE_SFF_8079_LEN;
+		}
+		/* Some I2C adapters cannot read 256 bytes in a single read.
+		 * Use a smaller chunk size to ensure we are within limits.
+		 */
+		len = min_t(unsigned int, len, 16);
+
+		ret = sfp_read(sfp, a2, this_addr, data, len);
 		if (ret < 0)
 			return ret;
 
 		first += len;
 		data += len;
 	}
-	if (first < ETH_MODULE_SFF_8472_LEN && last > ETH_MODULE_SFF_8079_LEN) {
-		len = min_t(unsigned int, last, ETH_MODULE_SFF_8472_LEN);
-		len -= first;
-		first -= ETH_MODULE_SFF_8079_LEN;
-
-		ret = sfp_read(sfp, true, first, data, len);
-		if (ret < 0)
-			return ret;
-	}
 	return 0;
 }
 
-- 
1.8.3.1

Re: [PATCH net-next] net: sfp: Use smaller chunk size when reading I2C data

[Russell King - ARM Linux admin]

On Fri, May 31, 2019 at 01:18:03PM -0600, Robert Hancock wrote:
> The SFP driver was reading up to 256 bytes of I2C data from the SFP
> module in a single chunk. However, some I2C controllers do not support
> reading that many bytes in a single transaction. Change to use a more
> compatible 16-byte chunk size, since this is not performance critical.

This is the wrong place to fix the problem.  We still end up reading
more than 16 bytes with this approach.  I already have a patch fixing
this the right way, which is in the queue to be sent.

> 
> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> ---
>  drivers/net/phy/sfp.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 6b6c83d..23a40a7 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -1651,7 +1651,7 @@ static int sfp_module_info(struct sfp *sfp, struct ethtool_modinfo *modinfo)
>  static int sfp_module_eeprom(struct sfp *sfp, struct ethtool_eeprom *ee,
>  			     u8 *data)
>  {
> -	unsigned int first, last, len;
> +	unsigned int first, last;
>  	int ret;
>  
>  	if (ee->len == 0)
> @@ -1659,26 +1659,36 @@ static int sfp_module_eeprom(struct sfp *sfp, struct ethtool_eeprom *ee,
>  
>  	first = ee->offset;
>  	last = ee->offset + ee->len;
> -	if (first < ETH_MODULE_SFF_8079_LEN) {
> -		len = min_t(unsigned int, last, ETH_MODULE_SFF_8079_LEN);
> -		len -= first;
>  
> -		ret = sfp_read(sfp, false, first, data, len);
> +	while (first < last) {
> +		bool a2;
> +		unsigned int this_addr, len;
> +
> +		if (first < ETH_MODULE_SFF_8079_LEN) {
> +			len = min_t(unsigned int, last,
> +				    ETH_MODULE_SFF_8079_LEN);
> +			len -= first;
> +			a2 = false;
> +			this_addr = first;
> +		} else {
> +			len = min_t(unsigned int, last,
> +				    ETH_MODULE_SFF_8472_LEN);
> +			len -= first;
> +			a2 = true;
> +			this_addr = first - ETH_MODULE_SFF_8079_LEN;
> +		}
> +		/* Some I2C adapters cannot read 256 bytes in a single read.
> +		 * Use a smaller chunk size to ensure we are within limits.
> +		 */
> +		len = min_t(unsigned int, len, 16);
> +
> +		ret = sfp_read(sfp, a2, this_addr, data, len);
>  		if (ret < 0)
>  			return ret;
>  
>  		first += len;
>  		data += len;
>  	}
> -	if (first < ETH_MODULE_SFF_8472_LEN && last > ETH_MODULE_SFF_8079_LEN) {
> -		len = min_t(unsigned int, last, ETH_MODULE_SFF_8472_LEN);
> -		len -= first;
> -		first -= ETH_MODULE_SFF_8079_LEN;
> -
> -		ret = sfp_read(sfp, true, first, data, len);
> -		if (ret < 0)
> -			return ret;
> -	}
>  	return 0;
>  }
>  
> -- 
> 1.8.3.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

[Robert Hancock]

Some copper SFP modules support both SGMII and 1000BaseX, but some
drivers/devices only support the 1000BaseX mode. Currently SGMII mode is
always being selected as the desired mode for such modules, and this
fails if the controller doesn't support SGMII. Add a fallback for this
case by trying 1000BaseX instead if the controller rejects SGMII mode.

Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
---
 drivers/net/phy/phylink.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 68d0a89..4fd72c2 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1626,6 +1626,7 @@ static int phylink_sfp_module_insert(void *upstream,
 {
 	struct phylink *pl = upstream;
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(orig_support) = { 0, };
 	struct phylink_link_state config;
 	phy_interface_t iface;
 	int ret = 0;
@@ -1635,6 +1636,7 @@ static int phylink_sfp_module_insert(void *upstream,
 	ASSERT_RTNL();
 
 	sfp_parse_support(pl->sfp_bus, id, support);
+	linkmode_copy(orig_support, support);
 	port = sfp_parse_port(pl->sfp_bus, id, support);
 
 	memset(&config, 0, sizeof(config));
@@ -1663,6 +1665,25 @@ static int phylink_sfp_module_insert(void *upstream,
 
 	config.interface = iface;
 	ret = phylink_validate(pl, support, &config);
+
+	if (ret && iface == PHY_INTERFACE_MODE_SGMII &&
+	    phylink_test(orig_support, 1000baseX_Full)) {
+		/* Copper modules may select SGMII but the interface may not
+		 * support that mode, try 1000BaseX if supported.
+		 */
+
+		netdev_warn(pl->netdev, "validation of %s/%s with support %*pb "
+			    "failed: %d, trying 1000BaseX\n",
+			    phylink_an_mode_str(MLO_AN_INBAND),
+			    phy_modes(config.interface),
+			    __ETHTOOL_LINK_MODE_MASK_NBITS, orig_support, ret);
+		iface = PHY_INTERFACE_MODE_1000BASEX;
+		config.interface = iface;
+		linkmode_copy(config.advertising, orig_support);
+		linkmode_copy(support, orig_support);
+		ret = phylink_validate(pl, support, &config);
+	}
+
 	if (ret) {
 		phylink_err(pl, "validation of %s/%s with support %*pb failed: %d\n",
 			    phylink_an_mode_str(MLO_AN_INBAND),
-- 
1.8.3.1

Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

[Russell King - ARM Linux admin]

On Fri, May 31, 2019 at 01:18:04PM -0600, Robert Hancock wrote:
> Some copper SFP modules support both SGMII and 1000BaseX,

The situation is way worse than that.  Some copper SFP modules are
programmed to support SGMII only.  Others are programmed to support
1000baseX only.  There is no way to tell from the EEPROM how they
are configured, and there is no way to auto-probe the format of the
control word (which is the difference between the two.)

> but some
> drivers/devices only support the 1000BaseX mode. Currently SGMII mode is
> always being selected as the desired mode for such modules, and this
> fails if the controller doesn't support SGMII. Add a fallback for this
> case by trying 1000BaseX instead if the controller rejects SGMII mode.

So, what happens when a controller supports both SGMII and 1000base-X
modes (such as the Marvell devices) but the module is setup for
1000base-X mode?

> 
> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> ---
>  drivers/net/phy/phylink.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 68d0a89..4fd72c2 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1626,6 +1626,7 @@ static int phylink_sfp_module_insert(void *upstream,
>  {
>  	struct phylink *pl = upstream;
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(orig_support) = { 0, };
>  	struct phylink_link_state config;
>  	phy_interface_t iface;
>  	int ret = 0;
> @@ -1635,6 +1636,7 @@ static int phylink_sfp_module_insert(void *upstream,
>  	ASSERT_RTNL();
>  
>  	sfp_parse_support(pl->sfp_bus, id, support);
> +	linkmode_copy(orig_support, support);
>  	port = sfp_parse_port(pl->sfp_bus, id, support);
>  
>  	memset(&config, 0, sizeof(config));
> @@ -1663,6 +1665,25 @@ static int phylink_sfp_module_insert(void *upstream,
>  
>  	config.interface = iface;
>  	ret = phylink_validate(pl, support, &config);
> +
> +	if (ret && iface == PHY_INTERFACE_MODE_SGMII &&
> +	    phylink_test(orig_support, 1000baseX_Full)) {
> +		/* Copper modules may select SGMII but the interface may not
> +		 * support that mode, try 1000BaseX if supported.
> +		 */

Here, you are talking about what the module itself supports, but this
code is determining what it should do based on what the _network
controller_ supports.

If the SFP module is programmed for SGMII, and the network controller
supports 1000base-X, then it isn't going to work very well - the
sender of the control word will be sending one format, and the
receiver will be interpreting the bits wrongly.

> +
> +		netdev_warn(pl->netdev, "validation of %s/%s with support %*pb "
> +			    "failed: %d, trying 1000BaseX\n",
> +			    phylink_an_mode_str(MLO_AN_INBAND),
> +			    phy_modes(config.interface),
> +			    __ETHTOOL_LINK_MODE_MASK_NBITS, orig_support, ret);
> +		iface = PHY_INTERFACE_MODE_1000BASEX;
> +		config.interface = iface;
> +		linkmode_copy(config.advertising, orig_support);
> +		linkmode_copy(support, orig_support);
> +		ret = phylink_validate(pl, support, &config);
> +	}
> +
>  	if (ret) {
>  		phylink_err(pl, "validation of %s/%s with support %*pb failed: %d\n",
>  			    phylink_an_mode_str(MLO_AN_INBAND),
> -- 
> 1.8.3.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

[Robert Hancock]

On 2019-05-31 2:18 p.m., Russell King - ARM Linux admin wrote:
> On Fri, May 31, 2019 at 01:18:04PM -0600, Robert Hancock wrote:
>> Some copper SFP modules support both SGMII and 1000BaseX,
> 
> The situation is way worse than that.  Some copper SFP modules are
> programmed to support SGMII only.  Others are programmed to support
> 1000baseX only.  There is no way to tell from the EEPROM how they
> are configured, and there is no way to auto-probe the format of the
> control word (which is the difference between the two.)
> 
>> but some
>> drivers/devices only support the 1000BaseX mode. Currently SGMII mode is
>> always being selected as the desired mode for such modules, and this
>> fails if the controller doesn't support SGMII. Add a fallback for this
>> case by trying 1000BaseX instead if the controller rejects SGMII mode.
> 
> So, what happens when a controller supports both SGMII and 1000base-X
> modes (such as the Marvell devices) but the module is setup for
> 1000base-X mode?

My description is likely a bit incorrect.. rather than supporting both
1000BaseX and SGMII, a given module can support either 1000BaseX or
SGMII, but likely only one at a time (at least without magic
vendor-specific commands to switch modes).

The logic in sfp_select_interface always selects SGMII mode for copper
modules, which is the preferred mode of operation since 100 and 10 Mbps
modes won't work with 1000BaseX. If the controller and module actually
support SGMII, everything is fine. If the controller doesn't support
SGMII, it should fail validation and the link won't come up. If the
module doesn't support SGMII, it may try to come up but the link likely
won't work properly.

Our device is mainly intended for fiber modules, which is why 1000BaseX
is being used. The variant of fiber modules we are using (for example,
Finisar FCLF8520P2BTL) are set up for 1000BaseX, and seem like they are
kind of a hack to allow using copper on devices which only support
1000BaseX mode (in fact that particular one is extra hacky since you
have to disable 1000BaseX autonegotiation on the host side). This patch
is basically intended to allow that particular case to work.

It's kind of a dumb situation, but in the absence of a way to tell from
the EEPROM content what mode the module is actually in (and you would
likely know better than I if there was), it seems like the best we can do.

> 
>>
>> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
>> ---
>>  drivers/net/phy/phylink.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 68d0a89..4fd72c2 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -1626,6 +1626,7 @@ static int phylink_sfp_module_insert(void *upstream,
>>  {
>>  	struct phylink *pl = upstream;
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(orig_support) = { 0, };
>>  	struct phylink_link_state config;
>>  	phy_interface_t iface;
>>  	int ret = 0;
>> @@ -1635,6 +1636,7 @@ static int phylink_sfp_module_insert(void *upstream,
>>  	ASSERT_RTNL();
>>  
>>  	sfp_parse_support(pl->sfp_bus, id, support);
>> +	linkmode_copy(orig_support, support);
>>  	port = sfp_parse_port(pl->sfp_bus, id, support);
>>  
>>  	memset(&config, 0, sizeof(config));
>> @@ -1663,6 +1665,25 @@ static int phylink_sfp_module_insert(void *upstream,
>>  
>>  	config.interface = iface;
>>  	ret = phylink_validate(pl, support, &config);
>> +
>> +	if (ret && iface == PHY_INTERFACE_MODE_SGMII &&
>> +	    phylink_test(orig_support, 1000baseX_Full)) {
>> +		/* Copper modules may select SGMII but the interface may not
>> +		 * support that mode, try 1000BaseX if supported.
>> +		 */
> 
> Here, you are talking about what the module itself supports, but this
> code is determining what it should do based on what the _network
> controller_ supports.

orig_support is from just after sfp_parse_support is called, so it's
reflecting everything we think the module supports. The "net: sfp: Set
1000BaseX support flag for 1000BaseT modules" patch adds 1000BaseX to
that list for copper modules, which allows this code to detect that
1000BaseX might be a possibility.

> 
> If the SFP module is programmed for SGMII, and the network controller
> supports 1000base-X, then it isn't going to work very well - the
> sender of the control word will be sending one format, and the
> receiver will be interpreting the bits wrongly.

Agreed, but as I mentioned above, it doesn't appear that there's any
sensible way to avoid that. Without this patch, both SGMII and 1000BaseX
copper modules would fail in a 1000BaseX-only controller. With this
patch in place, the 1000BaseX module will work.

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

[Russell King - ARM Linux admin]

On Fri, May 31, 2019 at 06:17:51PM -0600, Robert Hancock wrote:
> Our device is mainly intended for fiber modules, which is why 1000BaseX
> is being used. The variant of fiber modules we are using (for example,
> Finisar FCLF8520P2BTL) are set up for 1000BaseX, and seem like they are
> kind of a hack to allow using copper on devices which only support
> 1000BaseX mode (in fact that particular one is extra hacky since you
> have to disable 1000BaseX autonegotiation on the host side). This patch
> is basically intended to allow that particular case to work.

Looking at the data sheet for FCLF8520P2BTL, it explicit states:

PRODUCT SELECTION
Part Number	Link Indicator	1000BASE-X auto-negotiation
		on RX_LOS Pin	enabled by default
FCLF8520P2BTL	Yes		No
FCLF8521P2BTL	No		Yes
FCLF8522P2BTL	Yes		Yes

The idea being, you buy the correct one according to what the host
equipment requires, rather than just picking one and hoping it works.

The data sheet goes on to mention that the module uses a Marvell
88e1111 PHY, which seems to be quite common for copper SFPs from
multiple manufacturers (but not all) and is very flexible in how it
can be configured.

If we detect a PHY on the SFP module, we check detect whether it is
an 88e1111 PHY, and then read out its configured link type.  We don't
have a way to deal with the difference between FCLF8520P2BTL and
FCLF8521P2BTL, but at least we'll be able to tell whether we should
be in 1000Base-X mode for these modules, rather than SGMII.

For a SFP cage meant to support fiber, I would recommend using the
FCLF8521P2BTL or FCLF8522P2BTL since those will behave more like a
802.3z standards-compliant gigabit fiber connection.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

[Robert Hancock]

On 2019-06-02 9:15 a.m., Russell King - ARM Linux admin wrote:
> On Fri, May 31, 2019 at 06:17:51PM -0600, Robert Hancock wrote:
>> Our device is mainly intended for fiber modules, which is why 1000BaseX
>> is being used. The variant of fiber modules we are using (for example,
>> Finisar FCLF8520P2BTL) are set up for 1000BaseX, and seem like they are
>> kind of a hack to allow using copper on devices which only support
>> 1000BaseX mode (in fact that particular one is extra hacky since you
>> have to disable 1000BaseX autonegotiation on the host side). This patch
>> is basically intended to allow that particular case to work.
> 
> Looking at the data sheet for FCLF8520P2BTL, it explicit states:
> 
> PRODUCT SELECTION
> Part Number	Link Indicator	1000BASE-X auto-negotiation
> 		on RX_LOS Pin	enabled by default
> FCLF8520P2BTL	Yes		No
> FCLF8521P2BTL	No		Yes
> FCLF8522P2BTL	Yes		Yes
> 
> The idea being, you buy the correct one according to what the host
> equipment requires, rather than just picking one and hoping it works.
> 
> The data sheet goes on to mention that the module uses a Marvell
> 88e1111 PHY, which seems to be quite common for copper SFPs from
> multiple manufacturers (but not all) and is very flexible in how it
> can be configured.
> 
> If we detect a PHY on the SFP module, we check detect whether it is
> an 88e1111 PHY, and then read out its configured link type.  We don't
> have a way to deal with the difference between FCLF8520P2BTL and
> FCLF8521P2BTL, but at least we'll be able to tell whether we should
> be in 1000Base-X mode for these modules, rather than SGMII.

It looks like that might provide a solution for modules using the
Marvell PHY, however some of the modules we are supporting seem to use a
Broadcom PHY, and I have no idea if there is any documentation for those.

It would really be rather silly if there were absolutely no way to tell
what mode the module wants from the EEPROM.. I don't have any copper
modules set up for SGMII, but below is the ethtool -m output for two of
the 1000BaseX modules I have. If you have access to an SGMII module, can
you compare this to what it indicates?

# ethtool -m eth1
	Identifier                                : 0x03 (SFP)
	Extended identifier                       : 0x04 (GBIC/SFP defined by
2-wire interface ID)
	Connector                                 : 0x00 (unknown or unspecified)
	Transceiver codes                         : 0x00 0x00 0x00 0x08 0x00
0x00 0x00 0x00 0x00
	Transceiver type                          : Ethernet: 1000BASE-T
	Encoding                                  : 0x01 (8B/10B)
	BR, Nominal                               : 1200MBd
	Rate identifier                           : 0x00 (unspecified)
	Length (SMF,km)                           : 0km
	Length (SMF)                              : 0m
	Length (50um)                             : 0m
	Length (62.5um)                           : 0m
	Length (Copper)                           : 100m
	Length (OM3)                              : 0m
	Laser wavelength                          : 0nm
	Vendor name                               : FINISAR CORP.
	Vendor OUI                                : 00:90:65
	Vendor PN                                 : FCLF8520P2BTL
	Vendor rev                                : A
	Option values                             : 0x00 0x12
	Option                                    : RX_LOS implemented
	Option                                    : TX_DISABLE implemented
	BR margin, max                            : 0%
	BR margin, min                            : 0%
	Vendor SN                                 : PX90NHX
	Date code                                 : 170303


# ethtool -m eth1
	Identifier                                : 0x03 (SFP)
	Extended identifier                       : 0x04 (GBIC/SFP defined by
2-wire interface ID)
	Connector                                 : 0x00 (unknown or unspecified)
	Transceiver codes                         : 0x00 0x00 0x00 0x08 0x00
0x00 0x00 0x00 0x00
	Transceiver type                          : Ethernet: 1000BASE-T
	Encoding                                  : 0x01 (8B/10B)
	BR, Nominal                               : 1300MBd
	Rate identifier                           : 0x00 (unspecified)
	Length (SMF,km)                           : 0km
	Length (SMF)                              : 0m
	Length (50um)                             : 0m
	Length (62.5um)                           : 0m
	Length (Copper)                           : 100m
	Length (OM3)                              : 0m
	Laser wavelength                          : 0nm
	Vendor name                               : BEL-FUSE
	Vendor OUI                                : 00:00:00
	Vendor PN                                 : 1GBT-SFP06
	Vendor rev                                : PB
	Option values                             : 0x00 0x12
	Option                                    : RX_LOS implemented
	Option                                    : TX_DISABLE implemented
	BR margin, max                            : 0%
	BR margin, min                            : 0%
	Vendor SN                                 :      0000000610
	Date code                                 : 1336


> 
> For a SFP cage meant to support fiber, I would recommend using the
> FCLF8521P2BTL or FCLF8522P2BTL since those will behave more like a
> 802.3z standards-compliant gigabit fiber connection.
> 

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

[Russell King - ARM Linux admin]

On Fri, Jun 07, 2019 at 12:10:57PM -0600, Robert Hancock wrote:
> On 2019-06-02 9:15 a.m., Russell King - ARM Linux admin wrote:
> > On Fri, May 31, 2019 at 06:17:51PM -0600, Robert Hancock wrote:
> >> Our device is mainly intended for fiber modules, which is why 1000BaseX
> >> is being used. The variant of fiber modules we are using (for example,
> >> Finisar FCLF8520P2BTL) are set up for 1000BaseX, and seem like they are
> >> kind of a hack to allow using copper on devices which only support
> >> 1000BaseX mode (in fact that particular one is extra hacky since you
> >> have to disable 1000BaseX autonegotiation on the host side). This patch
> >> is basically intended to allow that particular case to work.
> > 
> > Looking at the data sheet for FCLF8520P2BTL, it explicit states:
> > 
> > PRODUCT SELECTION
> > Part Number	Link Indicator	1000BASE-X auto-negotiation
> > 		on RX_LOS Pin	enabled by default
> > FCLF8520P2BTL	Yes		No
> > FCLF8521P2BTL	No		Yes
> > FCLF8522P2BTL	Yes		Yes
> > 
> > The idea being, you buy the correct one according to what the host
> > equipment requires, rather than just picking one and hoping it works.
> > 
> > The data sheet goes on to mention that the module uses a Marvell
> > 88e1111 PHY, which seems to be quite common for copper SFPs from
> > multiple manufacturers (but not all) and is very flexible in how it
> > can be configured.
> > 
> > If we detect a PHY on the SFP module, we check detect whether it is
> > an 88e1111 PHY, and then read out its configured link type.  We don't
> > have a way to deal with the difference between FCLF8520P2BTL and
> > FCLF8521P2BTL, but at least we'll be able to tell whether we should
> > be in 1000Base-X mode for these modules, rather than SGMII.
> 
> It looks like that might provide a solution for modules using the
> Marvell PHY, however some of the modules we are supporting seem to use a
> Broadcom PHY, and I have no idea if there is any documentation for those.
> 
> It would really be rather silly if there were absolutely no way to tell
> what mode the module wants from the EEPROM..

It is something I've spent weeks looking at from many different angles.
There is no way to tell.

You have to bear in mind that 1000BaseX and SGMII are essentially
identical, except for the interpretation of that 16-bit control word
and how it is handled.  Both are 1250Mbaud, both are 8b/10b encoded.
Both identify as supporting 1000BASE-T.

As I've said, the only way I can come up with is a hard-coded table
of vendor name/part number to identify what each one requires.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

[Sergei Shtylyov]

Hello!

On 31.05.2019 22:18, Robert Hancock wrote:

> Some copper SFP modules support both SGMII and 1000BaseX, but some
> drivers/devices only support the 1000BaseX mode. Currently SGMII mode is
> always being selected as the desired mode for such modules, and this
> fails if the controller doesn't support SGMII. Add a fallback for this
> case by trying 1000BaseX instead if the controller rejects SGMII mode.
> 
> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> ---
>   drivers/net/phy/phylink.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 68d0a89..4fd72c2 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[...]
> @@ -1663,6 +1665,25 @@ static int phylink_sfp_module_insert(void *upstream,
>   
>   	config.interface = iface;
>   	ret = phylink_validate(pl, support, &config);
> +
> +	if (ret && iface == PHY_INTERFACE_MODE_SGMII &&
> +	    phylink_test(orig_support, 1000baseX_Full)) {
> +		/* Copper modules may select SGMII but the interface may not
> +		 * support that mode, try 1000BaseX if supported.
> +		 */
> +
> +		netdev_warn(pl->netdev, "validation of %s/%s with support %*pb "
> +			    "failed: %d, trying 1000BaseX\n",

    Don't break the messages like this, scripts/checkpatch.pl shouldn't 
complain about too long lines in this case.

[...]

MBR, Sergei

[PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[Robert Hancock]

The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX
mode in a somewhat unusual manner: it still exposes a PHY device which
needs some PHY-level initialization for the PCS/PMA layer to work properly,
and which provides some link status/control information.

In this case, we want to use the phylink layer to support proper
communication with the SFP module, but in most other respects we want to
use the PHY attached to the controller.

Currently the phylink driver does not initialize or use a controller PHY
even if it exists for fixed-link or 802.3z PHY modes, and doesn't
support SFP module attachment in those modes. This change allows it to
utilize a controller PHY if it is defined, and allows SFP module
attachment/initialization but does not connect the PHY device to the
controller (to allow the controller PHY to be used for link state
tracking).

Fully supporting this setup would probably require initializing and
tracking the state of both PHYs, which is a much more complex change and
doesn't appear to be required for this use case.

Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
---
 drivers/net/phy/phylink.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4fd72c2..9362aca 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -819,12 +819,6 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 	struct phy_device *phy_dev;
 	int ret;
 
-	/* Fixed links and 802.3z are handled without needing a PHY */
-	if (pl->link_an_mode == MLO_AN_FIXED ||
-	    (pl->link_an_mode == MLO_AN_INBAND &&
-	     phy_interface_mode_is_8023z(pl->link_interface)))
-		return 0;
-
 	phy_node = of_parse_phandle(dn, "phy-handle", 0);
 	if (!phy_node)
 		phy_node = of_parse_phandle(dn, "phy", 0);
@@ -1697,9 +1691,6 @@ static int phylink_sfp_module_insert(void *upstream,
 		    phy_modes(config.interface),
 		    __ETHTOOL_LINK_MODE_MASK_NBITS, support);
 
-	if (phy_interface_mode_is_8023z(iface) && pl->phydev)
-		return -EINVAL;
-
 	changed = !bitmap_equal(pl->supported, support,
 				__ETHTOOL_LINK_MODE_MASK_NBITS);
 	if (changed) {
@@ -1751,12 +1742,30 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phylink *pl = upstream;
 
+	/* In fixed mode, or in in-band mode with 802.3z PHY interface mode,
+	 *  ignore the SFP PHY and just use the PHY attached to the MAC.
+	 */
+	if (pl->link_an_mode == MLO_AN_FIXED ||
+	    (pl->link_an_mode == MLO_AN_INBAND &&
+	      phy_interface_mode_is_8023z(pl->link_config.interface)))
+		return 0;
+
 	return __phylink_connect_phy(upstream, phy, pl->link_config.interface);
 }
 
 static void phylink_sfp_disconnect_phy(void *upstream)
 {
-	phylink_disconnect_phy(upstream);
+	struct phylink *pl = upstream;
+
+	/* In fixed mode, or in in-band mode with 802.3z PHY interface mode,
+	 * ignore the SFP PHY and just use the PHY attached to the MAC.
+	 */
+	if (pl->link_an_mode == MLO_AN_FIXED ||
+	    (pl->link_an_mode == MLO_AN_INBAND &&
+	      phy_interface_mode_is_8023z(pl->link_config.interface)))
+		return;
+
+	phylink_disconnect_phy(pl);
 }
 
 static const struct sfp_upstream_ops sfp_phylink_ops = {
-- 
1.8.3.1

Re: [PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[David Miller]

So many changes to the same file that seem somehow related.

Therefore, why didn't you put this stuff into a formal patch series?

Re: [PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[Robert Hancock]

On 2019-05-31 1:29 p.m., David Miller wrote:
> 
> So many changes to the same file that seem somehow related.
> 
> Therefore, why didn't you put this stuff into a formal patch series?

The phy/phydev patches I submitted should all be independent changes
that aren't directly related or go in any particular order. (Though I
did notice I somehow managed to make some of them be reply chained to
others - whoops..)

They could potentially be a series though.

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[David Miller]

From: Robert Hancock <hancock@sedsystems.ca>
Date: Fri, 31 May 2019 13:41:05 -0600

> On 2019-05-31 1:29 p.m., David Miller wrote:
>> 
>> So many changes to the same file that seem somehow related.
>> 
>> Therefore, why didn't you put this stuff into a formal patch series?
> 
> The phy/phydev patches I submitted should all be independent changes
> that aren't directly related or go in any particular order. (Though I
> did notice I somehow managed to make some of them be reply chained to
> others - whoops..)
> 
> They could potentially be a series though.

If they are truly independent I guess it's ok.

Re: [PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[Russell King - ARM Linux admin]

On Fri, May 31, 2019 at 01:18:05PM -0600, Robert Hancock wrote:
> The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX
> mode in a somewhat unusual manner: it still exposes a PHY device which
> needs some PHY-level initialization for the PCS/PMA layer to work properly,
> and which provides some link status/control information.
> 
> In this case, we want to use the phylink layer to support proper
> communication with the SFP module, but in most other respects we want to
> use the PHY attached to the controller.
> 
> Currently the phylink driver does not initialize or use a controller PHY
> even if it exists for fixed-link or 802.3z PHY modes, and doesn't
> support SFP module attachment in those modes.

Sorry, I'm having a hard time following this description.  Please draw
an ASCII diagram of the setup you have - a picture is worth 1000 words,
and I think that is very much the case here.

We do have boards where the SFP is connected to a real PHY, where the
real PHY offers a RJ45 copper socket and a fiber interface,
automatically switching between the two.  In this case, we do not
use phylink to represent the link between the PHY and the SFP cage,
but instead the PHY binds directly to the SFP cage.

> This change allows it to
> utilize a controller PHY if it is defined, and allows SFP module
> attachment/initialization but does not connect the PHY device to the
> controller (to allow the controller PHY to be used for link state
> tracking).
> 
> Fully supporting this setup would probably require initializing and
> tracking the state of both PHYs, which is a much more complex change and
> doesn't appear to be required for this use case.
> 
> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> ---
>  drivers/net/phy/phylink.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 4fd72c2..9362aca 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -819,12 +819,6 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
>  	struct phy_device *phy_dev;
>  	int ret;
>  
> -	/* Fixed links and 802.3z are handled without needing a PHY */
> -	if (pl->link_an_mode == MLO_AN_FIXED ||
> -	    (pl->link_an_mode == MLO_AN_INBAND &&
> -	     phy_interface_mode_is_8023z(pl->link_interface)))
> -		return 0;
> -

This looks to me like it will break existing users.

>  	phy_node = of_parse_phandle(dn, "phy-handle", 0);
>  	if (!phy_node)
>  		phy_node = of_parse_phandle(dn, "phy", 0);
> @@ -1697,9 +1691,6 @@ static int phylink_sfp_module_insert(void *upstream,
>  		    phy_modes(config.interface),
>  		    __ETHTOOL_LINK_MODE_MASK_NBITS, support);
>  
> -	if (phy_interface_mode_is_8023z(iface) && pl->phydev)
> -		return -EINVAL;
> -
>  	changed = !bitmap_equal(pl->supported, support,
>  				__ETHTOOL_LINK_MODE_MASK_NBITS);
>  	if (changed) {
> @@ -1751,12 +1742,30 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  {
>  	struct phylink *pl = upstream;
>  
> +	/* In fixed mode, or in in-band mode with 802.3z PHY interface mode,
> +	 *  ignore the SFP PHY and just use the PHY attached to the MAC.
> +	 */
> +	if (pl->link_an_mode == MLO_AN_FIXED ||
> +	    (pl->link_an_mode == MLO_AN_INBAND &&
> +	      phy_interface_mode_is_8023z(pl->link_config.interface)))
> +		return 0;
> +
>  	return __phylink_connect_phy(upstream, phy, pl->link_config.interface);
>  }
>  
>  static void phylink_sfp_disconnect_phy(void *upstream)
>  {
> -	phylink_disconnect_phy(upstream);
> +	struct phylink *pl = upstream;
> +
> +	/* In fixed mode, or in in-band mode with 802.3z PHY interface mode,
> +	 * ignore the SFP PHY and just use the PHY attached to the MAC.
> +	 */
> +	if (pl->link_an_mode == MLO_AN_FIXED ||
> +	    (pl->link_an_mode == MLO_AN_INBAND &&
> +	      phy_interface_mode_is_8023z(pl->link_config.interface)))
> +		return;

Fixed link mode is mutually exclusive with there being a PHY present.
Please see Documentation/devicetree/bindings/net/fixed-link.txt

Fixed links are not used to fix a declared PHY to a specific mode.

> +
> +	phylink_disconnect_phy(pl);
>  }
>  
>  static const struct sfp_upstream_ops sfp_phylink_ops = {

Overall, I think you need to better describe what your setup is, and
what you are trying to achieve:

* Are you merely trying to support copper SFP modules where the PHY
  defaults to 1000base-X mode rather than SGMII?
* Are you trying to support a network controller that doesn't support
  SGMII mode?

If the former, then I'm pretty certain you're going about it the wrong
way - as I've said before, there is nothing in the EEPROM that
indicates definitively what format the control word is (and therefore
whether it is SGMII or 1000base-X.)

Some network controllers may be able to tell the difference, but that
is not true of all controllers.

The only way I can see to support such modules would be to have a table
of quirks to set the interface mode accordingly, and not try this "lets
pick one, try to validate it with the network controller, otherwise try
the other."

In any case, the format of the connection between the SFP module and
the network controller isn't one that should appear in the ethtool link
modes - I view what you've done there as a hack rather than proper
design.

If, on the other hand it is the latter, what you do you expect to
happen if you plug a copper SFP module that only supports SGMII into
a network controller that only supports 1000baseX ?  The PHY on some
of these modules won't pass data unless the SGMII handshake with the
network controller completes, which it may or may not do depending on
the 1000baseX implementation - but the network controller won't
interpret the bits correctly, and certainly won't be able to deal
with it when the link switches to 100M or 10M mode, which it will do
depending on the results of the copper side negotiation.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[Robert Hancock]

On 2019-05-31 2:31 p.m., Russell King - ARM Linux admin wrote:
> On Fri, May 31, 2019 at 01:18:05PM -0600, Robert Hancock wrote:
>> The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX
>> mode in a somewhat unusual manner: it still exposes a PHY device which
>> needs some PHY-level initialization for the PCS/PMA layer to work properly,
>> and which provides some link status/control information.
>>
>> In this case, we want to use the phylink layer to support proper
>> communication with the SFP module, but in most other respects we want to
>> use the PHY attached to the controller.
>>
>> Currently the phylink driver does not initialize or use a controller PHY
>> even if it exists for fixed-link or 802.3z PHY modes, and doesn't
>> support SFP module attachment in those modes.
> 
> Sorry, I'm having a hard time following this description.  Please draw
> an ASCII diagram of the setup you have - a picture is worth 1000 words,
> and I think that is very much the case here.
>
> We do have boards where the SFP is connected to a real PHY, where the
> real PHY offers a RJ45 copper socket and a fiber interface,
> automatically switching between the two.  In this case, we do not
> use phylink to represent the link between the PHY and the SFP cage,
> but instead the PHY binds directly to the SFP cage.
>

It sounds like the setup you're describing has the PHY being smarter and
doing more of the SFP module handling internally. In our setup, the
situation is something like this:

Xilinx MAC		I2C	GPIO
|
|GMII			|	|
|			|	|
Xilinx PHY		|	|
|			|	|
|1000BaseX		|	|
|			|	|
SFP -----------------------------

So in this case the Xilinx PHY is just really doing PCS/PMA, etc.
conversions. The I2C and GPIO lines for the SFP socket are routed back
to the host separately and the Xilinx PHY has no interaction with them
(other than that I believe the LOS signal from the SFP socket is
connected into the PHY to provide some link status indication back to it).

In this setup, phylink is basically allowing us to communicate with the
SFP module over I2C and manage the LOS, TX enable, etc. control lines
properly, but the Xilinx PHY does need to be initialized as well for the
actual link traffic to pass through.

>> This change allows it to
>> utilize a controller PHY if it is defined, and allows SFP module
>> attachment/initialization but does not connect the PHY device to the
>> controller (to allow the controller PHY to be used for link state
>> tracking).
>>
>> Fully supporting this setup would probably require initializing and
>> tracking the state of both PHYs, which is a much more complex change and
>> doesn't appear to be required for this use case.
>>
>> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
>> ---
>>  drivers/net/phy/phylink.c | 29 +++++++++++++++++++----------
>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 4fd72c2..9362aca 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -819,12 +819,6 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
>>  	struct phy_device *phy_dev;
>>  	int ret;
>>  
>> -	/* Fixed links and 802.3z are handled without needing a PHY */
>> -	if (pl->link_an_mode == MLO_AN_FIXED ||
>> -	    (pl->link_an_mode == MLO_AN_INBAND &&
>> -	     phy_interface_mode_is_8023z(pl->link_interface)))
>> -		return 0;
>> -
> 
> This looks to me like it will break existing users.
> 
>>  	phy_node = of_parse_phandle(dn, "phy-handle", 0);
>>  	if (!phy_node)
>>  		phy_node = of_parse_phandle(dn, "phy", 0);
>> @@ -1697,9 +1691,6 @@ static int phylink_sfp_module_insert(void *upstream,
>>  		    phy_modes(config.interface),
>>  		    __ETHTOOL_LINK_MODE_MASK_NBITS, support);
>>  
>> -	if (phy_interface_mode_is_8023z(iface) && pl->phydev)
>> -		return -EINVAL;
>> -
>>  	changed = !bitmap_equal(pl->supported, support,
>>  				__ETHTOOL_LINK_MODE_MASK_NBITS);
>>  	if (changed) {
>> @@ -1751,12 +1742,30 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>>  {
>>  	struct phylink *pl = upstream;
>>  
>> +	/* In fixed mode, or in in-band mode with 802.3z PHY interface mode,
>> +	 *  ignore the SFP PHY and just use the PHY attached to the MAC.
>> +	 */
>> +	if (pl->link_an_mode == MLO_AN_FIXED ||
>> +	    (pl->link_an_mode == MLO_AN_INBAND &&
>> +	      phy_interface_mode_is_8023z(pl->link_config.interface)))
>> +		return 0;
>> +
>>  	return __phylink_connect_phy(upstream, phy, pl->link_config.interface);
>>  }
>>  
>>  static void phylink_sfp_disconnect_phy(void *upstream)
>>  {
>> -	phylink_disconnect_phy(upstream);
>> +	struct phylink *pl = upstream;
>> +
>> +	/* In fixed mode, or in in-band mode with 802.3z PHY interface mode,
>> +	 * ignore the SFP PHY and just use the PHY attached to the MAC.
>> +	 */
>> +	if (pl->link_an_mode == MLO_AN_FIXED ||
>> +	    (pl->link_an_mode == MLO_AN_INBAND &&
>> +	      phy_interface_mode_is_8023z(pl->link_config.interface)))
>> +		return;
> 
> Fixed link mode is mutually exclusive with there being a PHY present.
> Please see Documentation/devicetree/bindings/net/fixed-link.txt
> 
> Fixed links are not used to fix a declared PHY to a specific mode.

I agree it would likely make sense to not include fixed mode in this case.

> 
>> +
>> +	phylink_disconnect_phy(pl);
>>  }
>>  
>>  static const struct sfp_upstream_ops sfp_phylink_ops = {
> 
> Overall, I think you need to better describe what your setup is, and
> what you are trying to achieve:
> 
> * Are you merely trying to support copper SFP modules where the PHY
>   defaults to 1000base-X mode rather than SGMII?
> * Are you trying to support a network controller that doesn't support
>   SGMII mode?

In our case the controller is supporting 1000BaseX only and is mainly
intended for fiber modules. We do want to be able to get copper modules
to work - obviously only ones that are set up for 1000BaseX mode are
possible.

> 
> If the former, then I'm pretty certain you're going about it the wrong
> way - as I've said before, there is nothing in the EEPROM that
> indicates definitively what format the control word is (and therefore
> whether it is SGMII or 1000base-X.)
> 
> Some network controllers may be able to tell the difference, but that
> is not true of all controllers.
> 
> The only way I can see to support such modules would be to have a table
> of quirks to set the interface mode accordingly, and not try this "lets
> pick one, try to validate it with the network controller, otherwise try
> the other."
> 
> In any case, the format of the connection between the SFP module and
> the network controller isn't one that should appear in the ethtool link
> modes - I view what you've done there as a hack rather than proper
> design.
> 
> If, on the other hand it is the latter, what you do you expect to
> happen if you plug a copper SFP module that only supports SGMII into
> a network controller that only supports 1000baseX ?  The PHY on some
> of these modules won't pass data unless the SGMII handshake with the
> network controller completes, which it may or may not do depending on
> the 1000baseX implementation - but the network controller won't
> interpret the bits correctly, and certainly won't be able to deal
> with it when the link switches to 100M or 10M mode, which it will do
> depending on the results of the copper side negotiation.
> 

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[Russell King - ARM Linux admin]

On Fri, May 31, 2019 at 06:33:32PM -0600, Robert Hancock wrote:
> On 2019-05-31 2:31 p.m., Russell King - ARM Linux admin wrote:
> > On Fri, May 31, 2019 at 01:18:05PM -0600, Robert Hancock wrote:
> >> The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX
> >> mode in a somewhat unusual manner: it still exposes a PHY device which
> >> needs some PHY-level initialization for the PCS/PMA layer to work properly,
> >> and which provides some link status/control information.
> >>
> >> In this case, we want to use the phylink layer to support proper
> >> communication with the SFP module, but in most other respects we want to
> >> use the PHY attached to the controller.
> >>
> >> Currently the phylink driver does not initialize or use a controller PHY
> >> even if it exists for fixed-link or 802.3z PHY modes, and doesn't
> >> support SFP module attachment in those modes.
> > 
> > Sorry, I'm having a hard time following this description.  Please draw
> > an ASCII diagram of the setup you have - a picture is worth 1000 words,
> > and I think that is very much the case here.
> >
> > We do have boards where the SFP is connected to a real PHY, where the
> > real PHY offers a RJ45 copper socket and a fiber interface,
> > automatically switching between the two.  In this case, we do not
> > use phylink to represent the link between the PHY and the SFP cage,
> > but instead the PHY binds directly to the SFP cage.
> >
> 
> It sounds like the setup you're describing has the PHY being smarter and
> doing more of the SFP module handling internally. In our setup, the
> situation is something like this:
> 
> Xilinx MAC		I2C	GPIO
> |
> |GMII			|	|
> |			|	|
> Xilinx PHY		|	|
> |			|	|
> |1000BaseX		|	|
> |			|	|
> SFP -----------------------------

That is very similar, except the Marvell 88x3310 uses a 10GBASE-R
protocol to a SFP+ module, but can be switched to either SGMII or
1000BASE-X mode (neither of which we currently support, but work is
in progress, if it turns out that these boards with strong pullups
can work with SFP modules.)

With the 88x3310, I have a couple of patches that "bolt on" to the
PHY driver, so we end up with this setup from the DT, kernel and
hardware point of view:

                 ,-----> Copper RJ45
   MAC -----> PHY
                 `-----> SFP

Hence, the PHY driver is responsible for registering itself as an
"upstream" of the SFP cage without involving phylink - phylink gets
used for the MAC <-> PHY part of the connection.

There's an awkward problem though: the PHY driver doesn't really have
much clue whether the network interface is up or down, which SFP
really needs to know so it can control whether the SFP module's laser
is emitting or not.  One of the patches tweaks the phylink code to
pass this information over to the SFP cage, around phylib, but the
proper solution would be for phylib to propagate that information to
the phylib driver, so that it can in turn pass that on to the SFP cage.

However, there's a slightly bigger problem looming here, which is that
phylib and the network layers in general do _not_ support having two
PHYs actively bound to one network interface, and without radically
reworking phylib and how phylib is bolted into the network layer, that
is not easy to get around.

> So in this case the Xilinx PHY is just really doing PCS/PMA, etc.
> conversions. The I2C and GPIO lines for the SFP socket are routed back
> to the host separately and the Xilinx PHY has no interaction with them
> (other than that I believe the LOS signal from the SFP socket is
> connected into the PHY to provide some link status indication back to it).

So, very similar situation as on the Macchiatobin with the 88x3310
PHYs.

> In this setup, phylink is basically allowing us to communicate with the
> SFP module over I2C and manage the LOS, TX enable, etc. control lines
> properly, but the Xilinx PHY does need to be initialized as well for the
> actual link traffic to pass through.

I think what you're missing is that the design is a layered one.
All the SFP cage stuff is interfaced through the sfp layer, and is
made accessible independently via the sfp-bus layer (which is needed
to avoid sfp being a hard dependency for the MAC driver - especially
important when we have SoCs such as Armada 8040 where one hardware
module provides multiple network ports.)

phylink provides a mechanism to separate PHYs from the MAC driver
such that we can hot-plug PHYs (necessary for the PHYs on SFP modules),
and deal with dynamically reconfiguring the MAC's hardware interface
mode according to what the module supports.  It isn't intended to
always be closely bound to the SFP cage side.

One of the reasons we have this design is because the early boards I
had access to when designing this setup were direct MAC to SFP cage
setups - there was no intermediate PHY.  Then came the Macchiatobin
board which does have a PHY, which brings with it additional
complexities, but various hardware problems have stood in the way of
having SFP modules in the 10G slots.

> In our case the controller is supporting 1000BaseX only and is mainly
> intended for fiber modules. We do want to be able to get copper modules
> to work - obviously only ones that are set up for 1000BaseX mode are
> possible.

So, what I say below applies:

> > If the former, then I'm pretty certain you're going about it the wrong
> > way - as I've said before, there is nothing in the EEPROM that
> > indicates definitively what format the control word is (and therefore
> > whether it is SGMII or 1000base-X.)
> > 
> > Some network controllers may be able to tell the difference, but that
> > is not true of all controllers.
> > 
> > The only way I can see to support such modules would be to have a table
> > of quirks to set the interface mode accordingly, and not try this "lets
> > pick one, try to validate it with the network controller, otherwise try
> > the other."
> > 
> > In any case, the format of the connection between the SFP module and
> > the network controller isn't one that should appear in the ethtool link
> > modes - I view what you've done there as a hack rather than proper
> > design.

I do have the beginnings of a quirk system for the sfp-bus layer,
since I've been conversing with someone with a GPON module that
does appear to follow the SFP MSA, in particular with regard to the
time it takes the module to start responding on I2C, and in regard
of the speeds it actually supports (basically, the EEPROM is
misleading.)  So, that should be useful for you as well.

http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy

is my playground of patches for SFP, which are in various stages of
maturity, some which have been posted for inclusion (and merged)
others that have been waiting some time.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode

[Robert Hancock]

On 2019-06-01 2:10 p.m., Russell King - ARM Linux admin wrote:
>>> Sorry, I'm having a hard time following this description.  Please draw
>>> an ASCII diagram of the setup you have - a picture is worth 1000 words,
>>> and I think that is very much the case here.
>>>
>>> We do have boards where the SFP is connected to a real PHY, where the
>>> real PHY offers a RJ45 copper socket and a fiber interface,
>>> automatically switching between the two.  In this case, we do not
>>> use phylink to represent the link between the PHY and the SFP cage,
>>> but instead the PHY binds directly to the SFP cage.
>>>
>>
>> It sounds like the setup you're describing has the PHY being smarter and
>> doing more of the SFP module handling internally. In our setup, the
>> situation is something like this:
>>
>> Xilinx MAC		I2C	GPIO
>> |
>> |GMII			|	|
>> |			|	|
>> Xilinx PHY		|	|
>> |			|	|
>> |1000BaseX		|	|
>> |			|	|
>> SFP -----------------------------
> 
> That is very similar, except the Marvell 88x3310 uses a 10GBASE-R
> protocol to a SFP+ module, but can be switched to either SGMII or
> 1000BASE-X mode (neither of which we currently support, but work is
> in progress, if it turns out that these boards with strong pullups
> can work with SFP modules.)
> 
> With the 88x3310, I have a couple of patches that "bolt on" to the
> PHY driver, so we end up with this setup from the DT, kernel and
> hardware point of view:
> 
>                  ,-----> Copper RJ45
>    MAC -----> PHY
>                  `-----> SFP
> 
> Hence, the PHY driver is responsible for registering itself as an
> "upstream" of the SFP cage without involving phylink - phylink gets
> used for the MAC <-> PHY part of the connection.

Looking at the patches you have on your branch, it looks like a similar
sort of approach could work in our case. One difference however, is that
the Marvell driver has its own internal PHY driver that knows about the
SFP cage attachment, whereas axienet doesnt (right now we are using the
generic PHY driver). Would it make sense for that SFP support to be
added into the generic PHY layer?

> 
> There's an awkward problem though: the PHY driver doesn't really have
> much clue whether the network interface is up or down, which SFP
> really needs to know so it can control whether the SFP module's laser
> is emitting or not.  One of the patches tweaks the phylink code to
> pass this information over to the SFP cage, around phylib, but the
> proper solution would be for phylib to propagate that information to
> the phylib driver, so that it can in turn pass that on to the SFP cage.
> 
> However, there's a slightly bigger problem looming here, which is that
> phylib and the network layers in general do _not_ support having two
> PHYs actively bound to one network interface, and without radically
> reworking phylib and how phylib is bolted into the network layer, that
> is not easy to get around.>
>> So in this case the Xilinx PHY is just really doing PCS/PMA, etc.
>> conversions. The I2C and GPIO lines for the SFP socket are routed back
>> to the host separately and the Xilinx PHY has no interaction with them
>> (other than that I believe the LOS signal from the SFP socket is
>> connected into the PHY to provide some link status indication back to it).
> 
> So, very similar situation as on the Macchiatobin with the 88x3310
> PHYs.
> 
>> In this setup, phylink is basically allowing us to communicate with the
>> SFP module over I2C and manage the LOS, TX enable, etc. control lines
>> properly, but the Xilinx PHY does need to be initialized as well for the
>> actual link traffic to pass through.
> 
> I think what you're missing is that the design is a layered one.
> All the SFP cage stuff is interfaced through the sfp layer, and is
> made accessible independently via the sfp-bus layer (which is needed
> to avoid sfp being a hard dependency for the MAC driver - especially
> important when we have SoCs such as Armada 8040 where one hardware
> module provides multiple network ports.)
> 
> phylink provides a mechanism to separate PHYs from the MAC driver
> such that we can hot-plug PHYs (necessary for the PHYs on SFP modules),
> and deal with dynamically reconfiguring the MAC's hardware interface
> mode according to what the module supports.  It isn't intended to
> always be closely bound to the SFP cage side.
> 
> One of the reasons we have this design is because the early boards I
> had access to when designing this setup were direct MAC to SFP cage
> setups - there was no intermediate PHY.  Then came the Macchiatobin
> board which does have a PHY, which brings with it additional
> complexities, but various hardware problems have stood in the way of
> having SFP modules in the 10G slots.
> 
>> In our case the controller is supporting 1000BaseX only and is mainly
>> intended for fiber modules. We do want to be able to get copper modules
>> to work - obviously only ones that are set up for 1000BaseX mode are
>> possible.
> 
> So, what I say below applies:
> 
>>> If the former, then I'm pretty certain you're going about it the wrong
>>> way - as I've said before, there is nothing in the EEPROM that
>>> indicates definitively what format the control word is (and therefore
>>> whether it is SGMII or 1000base-X.)
>>>
>>> Some network controllers may be able to tell the difference, but that
>>> is not true of all controllers.
>>>
>>> The only way I can see to support such modules would be to have a table
>>> of quirks to set the interface mode accordingly, and not try this "lets
>>> pick one, try to validate it with the network controller, otherwise try
>>> the other."
>>>
>>> In any case, the format of the connection between the SFP module and
>>> the network controller isn't one that should appear in the ethtool link
>>> modes - I view what you've done there as a hack rather than proper
>>> design.
> 
> I do have the beginnings of a quirk system for the sfp-bus layer,
> since I've been conversing with someone with a GPON module that
> does appear to follow the SFP MSA, in particular with regard to the
> time it takes the module to start responding on I2C, and in regard
> of the speeds it actually supports (basically, the EEPROM is
> misleading.)  So, that should be useful for you as well.
> 
> http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy
> 
> is my playground of patches for SFP, which are in various stages of
> maturity, some which have been posted for inclusion (and merged)
> others that have been waiting some time.
> 

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH net-next] net: sfp: Set 1000BaseX support flag for 1000BaseT modules

[Russell King - ARM Linux admin]

Sorry, didn't see this.

On Fri, May 31, 2019 at 01:18:01PM -0600, Robert Hancock wrote:
> Modules which support 1000BaseT should also have 1000BaseX support. Set
> this support flag to allow drivers supporting only 1000BaseX to work.
> 
> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> ---
>  drivers/net/phy/sfp-bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index e9c1879..96cdf85 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -158,6 +158,7 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
>  	if (id->base.e1000_base_t) {
>  		phylink_set(modes, 1000baseT_Half);
>  		phylink_set(modes, 1000baseT_Full);
> +		phylink_set(modes, 1000baseX_Full);

None of my RJ45 modules have 1000base-X support, in fact they use SGMII
on the host side and don't offer 1000base-X (fiber) on the connector
side.

Please explain the logic here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up