diff mbox series

phy: phylink: Fix CuSFP issue in phylink

Message ID 20201110100642.2153-1-bjarni.jonasson@microchip.com
State New
Headers show
Series phy: phylink: Fix CuSFP issue in phylink | expand

Commit Message

Bjarni Jonasson Nov. 10, 2020, 10:06 a.m. UTC
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(+)

Comments

Russell King - ARM Linux admin Nov. 10, 2020, 10:25 a.m. UTC | #1
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?
Bjarni Jonasson Nov. 10, 2020, 2:16 p.m. UTC | #2
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
Russell King - ARM Linux admin Nov. 10, 2020, 3:12 p.m. UTC | #3
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.
Bjarni Jonasson Nov. 11, 2020, 8:52 a.m. UTC | #4
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
Russell King - ARM Linux admin Nov. 15, 2020, 12:19 p.m. UTC | #5
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.
Bjarni Jonasson Nov. 17, 2020, 11:09 a.m. UTC | #6
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
Andrew Lunn Nov. 17, 2020, 1:45 p.m. UTC | #7
> > 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
Florian Fainelli Nov. 17, 2020, 3:08 p.m. UTC | #8
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?
diff mbox series

Patch

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;