Message-ID: <CAFSKS=N_SF-S35eL=tLWOQFKiq7YKKY5B9YT6kxZc0usBayE7w@mail.gmail.com>
Date:   Wed, 18 Sep 2019 13:44:33 -0500
From:   George McCollister <george.mccollister@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: SFP support with RGMII MAC via RGMII to SERDES/SGMII PHY?

Russell,

On Mon, Sep 16, 2019 at 10:40 AM George McCollister
<george.mccollister@...il.com> wrote:
>
> On Sat, Sep 14, 2019 at 3:49 AM Russell King - ARM Linux admin
> <linux@...linux.org.uk> wrote:
> >
> > On Fri, Sep 13, 2019 at 08:31:18PM -0700, Florian Fainelli wrote:
> > > +Russell, Andrew, Heiner,
> > >
> > > On 9/13/2019 9:44 AM, George McCollister wrote:
> > > > Every example of phylink SFP support I've seen is using an Ethernet
> > > > MAC with native SGMII.
> > > > Can phylink facilitate support of Fiber and Copper SFP modules
> > > > connected to an RGMII MAC if all of the following are true?
> > >
> > > I don't think that use case has been presented before, but phylink
> > > sounds like the tool that should help solve it. From your description
> > > below, it sounds like all the pieces are there to support it. Is the
> > > Ethernet MAC driver upstream?
> >
> > It has been presented, and it's something I've been trying to support
> > for the last couple of years - in fact, I have patches in my tree that
> > support a very similar scenario on the Macchiatobin with the 88x3310
> > PHYs.
> >
> > > > 1) The MAC is connected via RGMII to a transceiver/PHY (such as
> > > > Marvell 88E1512) which then connects to the SFP via SERDER/SGMII. If
> > > > you want to see a block diagram it's the first one here:
> > > > https://www.marvell.com/transceivers/assets/Alaska_88E1512-001_product_brief.pdf
> >
> > As mentioned above, this is no different from the Macchiatobin,
> > where we have:
> >
> >                   .-------- RJ45
> > MAC ---- 88x3310 PHY
> >                   `-------- SFP+
> >
> > except instead of the MAC to PHY link being 10GBASE-R, it's RGMII,
> > and the PHY to SFP+ link is 10GBASE-R instead of 1000BASE-X.

Did you test with an SFP+ module that has a PHY?

In my setup, nothing connects/attaches to the PHY (88E1111) in the
RJ45 SFP module I'm testing with (is this intended?). Apparently since
sfp_upstream_ops in the PHY driver used for the first PHY doesn't
provide a .connect_phy. This leaves .phy_link_change NULL. Eventually
phy_link_down tries to call .phy_link_change resulting in a NULL
pointer deference OOPs. If phy_link_change doesn't need to be called
for the second PHY I can just send a patch that doesn't call it if
it's NULL.

This is what I did:

I applied the following on top of net-next:
net: sfp: move fwnode parsing into sfp-bus layer
net: phy: provide phy driver start/stop hooks
net: phy: add core phylib sfp support

I then added SFP support to the marvell.c PHY driver like you did to
marvell10g.c. Just like with marvell10g.c I only provide .attach,
.detach and .module_insert sfp_upstream_ops.

I moved the sfp property from my Ethernet MAC to the PHY in DT (my DT
is a WIP and not upstream):
                ethphy1: ethernet-phy@1 {
                        reg = <1>;
+                       sfp = <&sfp>;
                };

Here is the console output, I've added some prints for debugging:

[    1.867336] sfp sfp: sfp_probe
[    1.870462] sfp sfp: sfp_probe - of_node
[    1.874520] libphy: SFP I2C Bus: probed
[    1.880290] sfp sfp: Host maximum power 2.0W
[    1.886681] sfp sfp: No tx_disable pin: SFP modules will always be emitting.
[    1.893771] sfp sfp: sfp_probe - returning 0
[    1.898392] libphy: Fixed MDIO Bus: probed
[    1.909454] fec 2188000.ethernet: Invalid MAC address: 00:00:00:00:00:00
[    1.916208] fec 2188000.ethernet: Using random MAC address: 62:2f:f1:55:56:dc
[    1.924075] libphy: fec_enet_mii_bus: probed
[    1.935382] libphy: phy_probe
[    1.938484] Marvell 88E1510 2188000.ethernet-1:00: marvell_probe
[    1.944561] Marvell 88E1510 2188000.ethernet-1:00: marvell_probe have fwnode
[    1.951688] sfp_register_upstream_node
[    1.955500] Marvell 88E1510 2188000.ethernet-1:00: sfp_bus = 00000000
[    1.968648] libphy: phy_probe
[    1.971745] Marvell 88E1510 2188000.ethernet-1:01: marvell_probe
[    1.977828] Marvell 88E1510 2188000.ethernet-1:01: marvell_probe have fwnode
[    1.984953] sfp_register_upstream_node
[    1.988895] sfp sfp: SM: enter empty:down:down event insert
[    1.994536] sfp sfp: SM: attached
[    1.997915] Marvell 88E1510 2188000.ethernet-1:01: marvell_sfp_attach
[    2.004415] Marvell 88E1510 2188000.ethernet-1:01: sfp_bus = de0ca240
[    2.012185] fec 2188000.ethernet eth0: registered PHC device 0
[    2.024656] fec 21b4000.ethernet: Invalid MAC address: 00:00:00:00:00:00
[    2.031461] fec 21b4000.ethernet: Using random MAC address: ce:e8:ce:0a:4e:3c
[    2.246517] libphy: fec_enet_mii_bus: probed
[    2.251440] fec 21b4000.ethernet eth1: registered PHC device 1
[    2.357150] sfp sfp: module OEM              SFP-GE-T         rev
   sn CSGETJ53492      dc 19050101
[    2.366568] Marvell 88E1510 2188000.ethernet-1:01: marvell_sfp_insert
[    2.373093] sfp sfp: Unknown/unsupported extended compliance code: 0x01

# ifconfig eth1 192.168.0.1
[   70.011960] libphy: phy_connect_direct
[   70.015830] Marvell 88E1510 2188000.ethernet-1:01: phy_attach_direct
[   70.025315] Marvell 88E1510 2188000.ethernet-1:01: marvell_config_init
[   70.034271] libphy: phy_resume
[   70.037955] Marvell 88E1510 2188000.ethernet-1:01: attached PHY
driver [Marvell 88E1510] (mii_bus:phy_addr=2188000.ethernet-1:01,
irq=POLL)
[   70.050591] Marvell 88E1510 2188000.ethernet-1:01: phy_start
[   70.060101] sfp sfp: SM: enter present:down:down event dev_up
[   70.131133] libphy: phy_probe
[   70.139706] Marvell 88E1111 i2c:sfp:16: marvell_probe
[   70.155806] Marvell 88E1111 i2c:sfp:16: sfp_add_phy - didnt call
connect_phy, ops = c0a4ca10
[   70.172962] sfp sfp: sfp_sm_probe_phy - calling phy_start
[   70.183163] Marvell 88E1111 i2c:sfp:16: phy_start
[   70.190973] Marvell 88E1111 i2c:sfp:16: phy_state_machine state = UP
[   70.197994] Marvell 88E1510 2188000.ethernet-1:01:
phy_state_machine state = UP
[   70.213607] Marvell 88E1510 2188000.ethernet-1:01: phy_start_aneg -
state = UP
[   70.225367] Marvell 88E1510 2188000.ethernet-1:01: phy_link_down -
calling phy_link_change
[   70.233864] Marvell 88E1510 2188000.ethernet-1:01: phy_link_change
[   70.243735] Marvell 88E1510 2188000.ethernet-1:01: PHY state change
UP -> NOLINK
[   70.251805] Marvell 88E1111 i2c:sfp:16: phy_start_aneg - state = UP
[   70.266411] Marvell 88E1111 i2c:sfp:16: phy_link_down -
phy_link_change is NULL and would be called !!!!!!!!!!!!!

It would have attempted to call the NULL .phy_link_change here but I
added a check to prevent it.

When I plug in the RJ45 cable it comes up and I'm able to ping.
[   88.282725] Marvell 88E1510 2188000.ethernet-1:01: phy_link_up -
calling phy_link_change
[   88.294628] Marvell 88E1510 2188000.ethernet-1:01: phy_link_change
[   88.303103] fec 21b4000.ethernet eth1: Link is Up - 1Gbps/Full -
flow control off
[   88.312487] Marvell 88E1510 2188000.ethernet-1:01: PHY state change
NOLINK -> RUNNING
[   88.338803] Marvell 88E1111 i2c:sfp:16: phy_state_machine state = NOLINK
[   89.364015] Marvell 88E1111 i2c:sfp:16: phy_state_machine state = NOLINK
[   89.372943] Marvell 88E1510 2188000.ethernet-1:01:
phy_state_machine state = RUNNING
[   89.386467] Marvell 88E1111 i2c:sfp:16: phy_link_up -
phy_link_change is NULL and would be called !!!!!!!!!!!!!!!

It would have attempted to call the NULL .phy_link_change again here too.

[   89.394896] Marvell 88E1111 i2c:sfp:16: PHY state change NOLINK -> RUNNING

> >
> > Note that you're abusing the term "SGMII".  SGMII is a Cisco
> > modification of the IEEE 802.3 1000BASE-X protocol.  Fiber SFPs
> > exclusively use 1000BASE-X protocol.  However, some copper SFPs
> > (with a RJ45) do use SGMII.
> >
> > > > 2) The 1G Ethernet driver has been converted to use phylink.
> >
> > This is not necessary for this scenario.  The PHY driver needs to
> > be updated to know about SFP though.
>
> Excellent, this is exactly the information I was looking for. I had
> started converting the Ethernet driver to phylink but there was still
> a lot of work to do and I fear there was a significant potential for
> regressions. I'll abandon that and see if I can get it to work by
> making similar changes to the 1G Marvell PHY driver.
>
> I'm assuming I must set the sfp property of the PHY in DT instead of the MAC.
>
> >
> > See:
> >
> > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=phy&id=ece56785ee0e9df40dc823fdc39ee74b4a7cd1c4
> >
> > as an example of the 88x3310 supporting a SFP+ cage.  This patch is
> > also necessary:
> >
> > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=phy&id=ef2d699397ca28c7f89e01cc9e5037989096a990
>
> Perfect.
>
> >
> > and if anything is going to stand in the way of progress on this, it
> > is likely to be that patch.  I'll be attempting to post these after
> > the next merge window (i.o.w. probably posting them in three weeks
> > time.)
>
> Please CC me on these patches as I'll follow your lead for what I do
> on the 1G Marvell PHY driver. If you don't remember, that's fine, I
> understand.
>
> >
> > --
> > 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
>
> Thanks,
> George

Regards,
George

Powered by blists - more mailing lists