[PATCH] phy: phylink: Fix CuSFP issue in phylink

[PATCH] phy: phylink: Fix CuSFP issue in phylink

[Bjarni Jonasson]

There is an issue with the current phylink driver and CuSFPs which
results in a callback to the phylink validate function without any
advertisement capabilities.  The workaround (in this changeset)
is to assign capabilities if a 1000baseT SFP is identified.

Signed-off-by: Bjarni Jonasson <bjarni.jonasson@microchip.com>
---
 drivers/net/phy/phylink.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 32f4e8ec96cf..76e25f7f6934 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2196,6 +2196,14 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 		mode = MLO_AN_INBAND;
 
 	/* Do the initial configuration */
+	if (phylink_test(pl->sfp_support, 1000baseT_Full)) {
+		pr_info("%s:%d: adding 1000baseT to PHY\n", __func__, __LINE__);
+		phylink_set(phy->supported, 1000baseT_Half);
+		phylink_set(phy->supported, 1000baseT_Full);
+		phylink_set(phy->advertising, 1000baseT_Half);
+		phylink_set(phy->advertising, 1000baseT_Full);
+	}
+
 	ret = phylink_sfp_config(pl, mode, phy->supported, phy->advertising);
 	if (ret < 0)
 		return ret;
-- 
2.17.1

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Russell King - ARM Linux admin]

On Tue, Nov 10, 2020 at 11:06:42AM +0100, Bjarni Jonasson wrote:
> There is an issue with the current phylink driver and CuSFPs which
> results in a callback to the phylink validate function without any
> advertisement capabilities.  The workaround (in this changeset)
> is to assign capabilities if a 1000baseT SFP is identified.

How does this happen?  Which PHY is being used?

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

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Bjarni Jonasson]

Russell King - ARM Linux admin writes:

> On Tue, Nov 10, 2020 at 11:06:42AM +0100, Bjarni Jonasson wrote:
>> There is an issue with the current phylink driver and CuSFPs which
>> results in a callback to the phylink validate function without any
>> advertisement capabilities.  The workaround (in this changeset)
>> is to assign capabilities if a 1000baseT SFP is identified.
>
> How does this happen?  Which PHY is being used?

This occurs just by plugging in the CuSFP.
None of the CuSFPs we have tested are working.
This is a dump from 3 different CuSFPs, phy regs 0-3:
FS SFP: 01:40:79:49 
HP SFP: 01:40:01:49
Marvel SFP: 01:40:01:49
This was working before the delayed mac config was implemented (in dec
2019).

--
Bjarni Jonasson, Microchip

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Russell King - ARM Linux admin]

On Tue, Nov 10, 2020 at 03:16:34PM +0100, Bjarni Jonasson wrote:
> 
> Russell King - ARM Linux admin writes:
> 
> > On Tue, Nov 10, 2020 at 11:06:42AM +0100, Bjarni Jonasson wrote:
> >> There is an issue with the current phylink driver and CuSFPs which
> >> results in a callback to the phylink validate function without any
> >> advertisement capabilities.  The workaround (in this changeset)
> >> is to assign capabilities if a 1000baseT SFP is identified.
> >
> > How does this happen?  Which PHY is being used?
> 
> This occurs just by plugging in the CuSFP.
> None of the CuSFPs we have tested are working.
> This is a dump from 3 different CuSFPs, phy regs 0-3:
> FS SFP: 01:40:79:49 
> HP SFP: 01:40:01:49
> Marvel SFP: 01:40:01:49
> This was working before the delayed mac config was implemented (in dec
> 2019).

You're dumping PHY registers 0 and 1 there, not 0 through 3, which
the values confirm. I don't recognise the format either. PHY registers
are always 16-bit.

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

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Bjarni Jonasson]

Russell King - ARM Linux admin writes:

> On Tue, Nov 10, 2020 at 03:16:34PM +0100, Bjarni Jonasson wrote:
>>
>> Russell King - ARM Linux admin writes:
>>
>> > On Tue, Nov 10, 2020 at 11:06:42AM +0100, Bjarni Jonasson wrote:
>> >> There is an issue with the current phylink driver and CuSFPs which
>> >> results in a callback to the phylink validate function without any
>> >> advertisement capabilities.  The workaround (in this changeset)
>> >> is to assign capabilities if a 1000baseT SFP is identified.
>> >
>> > How does this happen?  Which PHY is being used?
>>
>> This occurs just by plugging in the CuSFP.
>> None of the CuSFPs we have tested are working.
>> This is a dump from 3 different CuSFPs, phy regs 0-3:
>> FS SFP: 01:40:79:49
>> HP SFP: 01:40:01:49
>> Marvel SFP: 01:40:01:49
>> This was working before the delayed mac config was implemented (in dec
>> 2019).
>
> You're dumping PHY registers 0 and 1 there, not 0 through 3, which
> the values confirm. I don't recognise the format either. PHY registers
> are always 16-bit.
Sorry about that. Here is it again:
Marvell SFP : 0x0140 0x0149 0x0141 0x0cc1
FS SFP      : 0x1140 0x7949 0x0141 0x0cc2
Cisco SFP   : 0x0140 0x0149 0x0141 0x0cc1
I.e. its seems to be a Marvell phy (0x0141) in all cases.
And this occurs when phylink_start() is called.
--
Bjarni Jonasson, Microchip

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Russell King - ARM Linux admin]

On Wed, Nov 11, 2020 at 09:52:18AM +0100, Bjarni Jonasson wrote:
> 
> Russell King - ARM Linux admin writes:
> 
> > On Tue, Nov 10, 2020 at 03:16:34PM +0100, Bjarni Jonasson wrote:
> >>
> >> Russell King - ARM Linux admin writes:
> >>
> >> > On Tue, Nov 10, 2020 at 11:06:42AM +0100, Bjarni Jonasson wrote:
> >> >> There is an issue with the current phylink driver and CuSFPs which
> >> >> results in a callback to the phylink validate function without any
> >> >> advertisement capabilities.  The workaround (in this changeset)
> >> >> is to assign capabilities if a 1000baseT SFP is identified.
> >> >
> >> > How does this happen?  Which PHY is being used?
> >>
> >> This occurs just by plugging in the CuSFP.
> >> None of the CuSFPs we have tested are working.
> >> This is a dump from 3 different CuSFPs, phy regs 0-3:
> >> FS SFP: 01:40:79:49
> >> HP SFP: 01:40:01:49
> >> Marvel SFP: 01:40:01:49
> >> This was working before the delayed mac config was implemented (in dec
> >> 2019).
> >
> > You're dumping PHY registers 0 and 1 there, not 0 through 3, which
> > the values confirm. I don't recognise the format either. PHY registers
> > are always 16-bit.
> Sorry about that. Here is it again:
> Marvell SFP : 0x0140 0x0149 0x0141 0x0cc1
> FS SFP      : 0x1140 0x7949 0x0141 0x0cc2
> Cisco SFP   : 0x0140 0x0149 0x0141 0x0cc1
> I.e. its seems to be a Marvell phy (0x0141) in all cases.
> And this occurs when phylink_start() is called.

So they're all 88E1111 devices, which is the most common PHY for
CuSFPs.

Do you have the Marvell PHY driver either built-in or available as a
module? I suspect the problem is you don't. You will need the Marvell
PHY driver to correctly drive the PHY, you can't rely on the fallback
driver for SFPs.

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

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Bjarni Jonasson]

Russell King - ARM Linux admin writes:

> On Wed, Nov 11, 2020 at 09:52:18AM +0100, Bjarni Jonasson wrote:
>>
>> Russell King - ARM Linux admin writes:
>>
>> > On Tue, Nov 10, 2020 at 03:16:34PM +0100, Bjarni Jonasson wrote:
>> >>
>> >> Russell King - ARM Linux admin writes:
>> >>
>> >> > On Tue, Nov 10, 2020 at 11:06:42AM +0100, Bjarni Jonasson wrote:
>> >> >> There is an issue with the current phylink driver and CuSFPs which
>> >> >> results in a callback to the phylink validate function without any
>> >> >> advertisement capabilities.  The workaround (in this changeset)
>> >> >> is to assign capabilities if a 1000baseT SFP is identified.
>> >> >
>> >> > How does this happen?  Which PHY is being used?
>> >>
>> >> This occurs just by plugging in the CuSFP.
>> >> None of the CuSFPs we have tested are working.
>> >> This is a dump from 3 different CuSFPs, phy regs 0-3:
>> >> FS SFP: 01:40:79:49
>> >> HP SFP: 01:40:01:49
>> >> Marvel SFP: 01:40:01:49
>> >> This was working before the delayed mac config was implemented (in dec
>> >> 2019).
>> >
>> > You're dumping PHY registers 0 and 1 there, not 0 through 3, which
>> > the values confirm. I don't recognise the format either. PHY registers
>> > are always 16-bit.
>> Sorry about that. Here is it again:
>> Marvell SFP : 0x0140 0x0149 0x0141 0x0cc1
>> FS SFP      : 0x1140 0x7949 0x0141 0x0cc2
>> Cisco SFP   : 0x0140 0x0149 0x0141 0x0cc1
>> I.e. its seems to be a Marvell phy (0x0141) in all cases.
>> And this occurs when phylink_start() is called.
>
> So they're all 88E1111 devices, which is the most common PHY for
> CuSFPs.
>
> Do you have the Marvell PHY driver either built-in or available as a
> module? I suspect the problem is you don't. You will need the Marvell
> PHY driver to correctly drive the PHY, you can't rely on the fallback
> driver for SFPs.
Correct.  I was using the generic driver and that does clearly not
work.  After including the Marvell driver the callback to the validate
function happens as expected.  Thanks for the support.
--
Bjarni Jonasson Microchip

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Andrew Lunn]

> > Do you have the Marvell PHY driver either built-in or available as a
> > module? I suspect the problem is you don't. You will need the Marvell
> > PHY driver to correctly drive the PHY, you can't rely on the fallback
> > driver for SFPs.
> Correct.  I was using the generic driver and that does clearly not
> work.  After including the Marvell driver the callback to the validate
> function happens as expected.  Thanks for the support.

Hi Russell

Maybe we should have MDIO_I2C driver select the Marvell PHY driver?

      Andrew

Re: [PATCH] phy: phylink: Fix CuSFP issue in phylink

[Florian Fainelli]

On 11/17/2020 5:45 AM, Andrew Lunn wrote:
>>> Do you have the Marvell PHY driver either built-in or available as a
>>> module? I suspect the problem is you don't. You will need the Marvell
>>> PHY driver to correctly drive the PHY, you can't rely on the fallback
>>> driver for SFPs.
>> Correct.  I was using the generic driver and that does clearly not
>> work.  After including the Marvell driver the callback to the validate
>> function happens as expected.  Thanks for the support.
> 
> Hi Russell
> 
> Maybe we should have MDIO_I2C driver select the Marvell PHY driver?

It was suggested a while ago that MARVELL_PHY follow the SFP
configuration symbol and that we would warn when a CuSFP module was used
with the Generic PHY driver:

https://www.mail-archive.com/netdev@vger.kernel.org/msg253839.html

Eventually we did not make progress towards creating a list of modules
that would require a specialized PHY driver, maybe we can start now?
-- 
Florian