* russell's net-queue question
@ 2020-10-20 15:15 Marek Behún
2020-10-20 15:45 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 3+ messages in thread
From: Marek Behún @ 2020-10-20 15:15 UTC (permalink / raw)
To: Russell King - ARM Linux admin; +Cc: netdev
Russell,
I think the following commits in your net-queue should be still made better:
7f79709b7a15 ("net: phy: pass supported PHY interface types to phylib")
eba49a289d09 ("net: phy: marvell10g: select host interface configuration")
http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=eba49a289d0959eab3dfbc0320334eb5a855ca68
http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=eba49a289d0959eab3dfbc0320334eb5a855ca68
The first one adds filling of the phydev->host_interfaces bitmap into
the phylink_sfp_connect_phy function. It should also fill this bitmap
in functions phylink_connect_phy and phylink_of_phy_connect (direct
copy of pl->config->supported_interfaces).
The reason is that phy devices may want to know what interfaces are
supported by host even if no SFP is used (Marvell 88X3310 is an exmaple
of this).
The second patch (adding mactype selection to marvell10g) can get rid
of the rate matching code, and also
should update the mv3310_update_interface code accordignly.
Should I sent you these patches updated or should I create new patches
on top of yours?
Marek
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: russell's net-queue question 2020-10-20 15:15 russell's net-queue question Marek Behún @ 2020-10-20 15:45 ` Russell King - ARM Linux admin 2020-10-21 13:48 ` Marek Behún 0 siblings, 1 reply; 3+ messages in thread From: Russell King - ARM Linux admin @ 2020-10-20 15:45 UTC (permalink / raw) To: Marek Behún; +Cc: netdev On Tue, Oct 20, 2020 at 05:15:39PM +0200, Marek Behún wrote: > Russell, > > I think the following commits in your net-queue should be still made better: > > 7f79709b7a15 ("net: phy: pass supported PHY interface types to phylib") > eba49a289d09 ("net: phy: marvell10g: select host interface configuration") > > http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=eba49a289d0959eab3dfbc0320334eb5a855ca68 > http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=eba49a289d0959eab3dfbc0320334eb5a855ca68 > > The first one adds filling of the phydev->host_interfaces bitmap into > the phylink_sfp_connect_phy function. It should also fill this bitmap > in functions phylink_connect_phy and phylink_of_phy_connect (direct > copy of pl->config->supported_interfaces). First, the whole way interfaces are handled is really not good, even with the addition of the interfaces bitmap. However, it tries to solve at least some of the issues. Secondly, what should we fill this in with? Do we fill it with the firmware specified phy-mode setting? Or all the capabilities of the network driver's interface? What if the network driver supports RGMII/SGMII/10GBASE-R/etc but not all of these are wired? We really don't want the PHY changing what was configured via hardware when it's "built in", because it's ambiguous in a very many situations which mode should be selected. If we take the view that the firmware specified phy-mode should only be specified, then the 88X3310 will switch to MACTYPE=6 instead of 4 on the Macchiatobin, which is the rate adaption mode - and this will lead to lost packets (it's a plain 88X3310 without the MACSEC, so the PHY is not capable of generating flow control packets to pace the host.) > The reason is that phy devices may want to know what interfaces are > supported by host even if no SFP is used (Marvell 88X3310 is an exmaple > of this). If a SFP is not being used, then the connectivity is described via DT and the hardware configuration of the PHY (which we rely on for the 88X3310.) I don't see much of a solution to that for the 88X3310. If DT describes the interface mode as 10gbase-r, then that ambiguously could refer to MACTYPE=4,5,6 - the driver can't know. So, I don't think there is a simple answer here. > The second patch (adding mactype selection to marvell10g) can get rid > of the rate matching code, and also > should update the mv3310_update_interface code accordignly. > > Should I sent you these patches updated or should I create new patches > on top of yours? These are experimental, and for the reasons I mention above, they need careful thought. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: russell's net-queue question 2020-10-20 15:45 ` Russell King - ARM Linux admin @ 2020-10-21 13:48 ` Marek Behún 0 siblings, 0 replies; 3+ messages in thread From: Marek Behún @ 2020-10-21 13:48 UTC (permalink / raw) To: Russell King - ARM Linux admin; +Cc: netdev On Tue, 20 Oct 2020 16:45:15 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Tue, Oct 20, 2020 at 05:15:39PM +0200, Marek Behún wrote: > > Russell, > > > > I think the following commits in your net-queue should be still made better: > > > > 7f79709b7a15 ("net: phy: pass supported PHY interface types to phylib") > > eba49a289d09 ("net: phy: marvell10g: select host interface configuration") > > > > http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=eba49a289d0959eab3dfbc0320334eb5a855ca68 > > http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=eba49a289d0959eab3dfbc0320334eb5a855ca68 > > > > The first one adds filling of the phydev->host_interfaces bitmap into > > the phylink_sfp_connect_phy function. It should also fill this bitmap > > in functions phylink_connect_phy and phylink_of_phy_connect (direct > > copy of pl->config->supported_interfaces). > > First, the whole way interfaces are handled is really not good, even > with the addition of the interfaces bitmap. However, it tries to solve > at least some of the issues. > > Secondly, what should we fill this in with? > > Do we fill it with the firmware specified phy-mode setting? Or all the > capabilities of the network driver's interface? What if the network > driver supports RGMII/SGMII/10GBASE-R/etc but not all of these are > wired? > > We really don't want the PHY changing what was configured via hardware > when it's "built in", because it's ambiguous in a very many situations > which mode should be selected. If we take the view that the firmware > specified phy-mode should only be specified, then the 88X3310 will > switch to MACTYPE=6 instead of 4 on the Macchiatobin, which is the rate > adaption mode - and this will lead to lost packets (it's a plain > 88X3310 without the MACSEC, so the PHY is not capable of generating > flow control packets to pace the host.) > I was thinking about filling it with all interfaces supported by that device (which, I confess, I meant to be all interfaces supported by the SOC). You are right though that this is problematic, at least because some of the pins will not be connected at all... And I guess it would be overkill if devicetree had to specify all interfaces. Although... maybe we could start interpreting the phy-mode from DT (for SFP ports) as max supported mode. Because there may be boards with SOCs where mac driver supports, for example, all of USXGMII, 10gbase-r, 5gbase-r, 2500base-x, 1000base-x. But the board was certified only up to 5gbase-r, for FCC purposes, or maybe there is a connector somewhere that guarantees only 5 GHz... > > The reason is that phy devices may want to know what interfaces are > > supported by host even if no SFP is used (Marvell 88X3310 is an exmaple > > of this). > > If a SFP is not being used, then the connectivity is described via DT > and the hardware configuration of the PHY (which we rely on for the > 88X3310.) I don't see much of a solution to that for the 88X3310. > If DT describes the interface mode as 10gbase-r, then that ambiguously > could refer to MACTYPE=4,5,6 - the driver can't know. > > So, I don't think there is a simple answer here. > > > The second patch (adding mactype selection to marvell10g) can get rid > > of the rate matching code, and also > > should update the mv3310_update_interface code accordignly. > > > > Should I sent you these patches updated or should I create new patches > > on top of yours? > > These are experimental, and for the reasons I mention above, they > need careful thought. > Very well, I think you are right. BTW, I also sent yesterday some patches for your net-queue branch (they are tagged "russell-kings-net-queue"). What do you think about those? Also the way marvell10g is in your net-queue may break 88E2110 currently. I have a device with this card, I shall look into this and maybe send you a patch for this. Marek ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-21 13:48 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-20 15:15 russell's net-queue question Marek Behún 2020-10-20 15:45 ` Russell King - ARM Linux admin 2020-10-21 13:48 ` Marek Behún
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).