| Message ID | 20201110100642.2153-1-bjarni.jonasson@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | phy: phylink: Fix CuSFP issue in phylink | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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?
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
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.
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
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.
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
> > 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
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 --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;
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(+)