| Message ID | 20201116111511.5061-5-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support for RollBall 10G copper SFP modules | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| 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 | warning | WARNING: From:/Signed-off-by: email name mismatch: 'From: "Marek Behún" <kabel@kernel.org>' != 'Signed-off-by: Marek Behún <kabel@kernel.org>' |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> 5gbase-r, 2500base-x ans SGMII depending on copper speed) if this is the
s/ans/and/
It would seem I have a consistent problem with this typo. Should I send
another version?
Marek
Hi Russell, previously you replied on this patch: > This'll do as a stop-gap until we have a better way to determine which > MACTYPE mode we should be using. Can we consider this as Acked-by ?
On Mon, Nov 16, 2020 at 03:45:52PM +0100, Marek Behún wrote: > Hi Russell, > > previously you replied on this patch: > > > This'll do as a stop-gap until we have a better way to determine which > > MACTYPE mode we should be using. > > Can we consider this as Acked-by ? Not really. The selection of MACTYPE isn't as simple as you make out in this patch. If we know that the MAC supports 2500BASE-X and/or SGMII, that means MACTYPES 0, 3, 4, 5 are possible to fit that, all likely will work if we restrict the PHY to either 2.5G only or 1G..10M only. However, it only becomes important if the faster speeds are supported at the MAC. I'm afraid I haven't put much thought into how to solve it, and as I'm totally demotivated at the moment, that's unlikely to change.
On Mon, 16 Nov 2020 15:02:16 +0000 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Mon, Nov 16, 2020 at 03:45:52PM +0100, Marek Behún wrote: > > Hi Russell, > > > > previously you replied on this patch: > > > > > This'll do as a stop-gap until we have a better way to determine which > > > MACTYPE mode we should be using. > > > > Can we consider this as Acked-by ? > > Not really. > > The selection of MACTYPE isn't as simple as you make out in this patch. Hi Russell, I know that. The idea behind this patch is to add support for at least something (for MACs supporting 1G/2.5G, but not 10G) in a simple way. Full support can be added later, since it requires changes into other subsystems (the experimental patches in your repo). The question therefore IMO is whether this will introduce regression or not. > If we know that the MAC supports 2500BASE-X and/or SGMII, that means > MACTYPES 0, 3, 4, 5 are possible to fit that, all likely will work if > we restrict the PHY to either 2.5G only or 1G..10M only. However, it > only becomes important if the faster speeds are supported at the MAC. OK, but by applying this patch a regression could possibly occur only if (and shouldn't anyway, see below): - MAC supports 10G mode, but only XAUI/RXAUI, not XFI - mactype is by default set to 1 (XAUI with rate matching) or 2 (RXAUI with rate matching) or 7 (USXGMII) - before config_init, phydev->interface mode is 2500base-x or sgmii The question is whether someone uses such a configuration and expects the PHY driver to change phydev->interface. Anyway a regression should not occur anyway (i.e. this patch should't break something that did work previously), because even if XAUI/RXAUI with rate matching or USXGMII was selected by default on the PHY, the mv3310_update_interface does not support this modes currently (only 10gbase-r, 2500base-x and sgmii). So unless the MAC driver ignores the changed phydev->interface, this patch should not break anything. If it does cause a regression in spite of the points above, we can condition the mactype change to occur only if the mactype before the change was 6 (XFI with rate matching). Or map the change like so: 1 -> 3 XAUI with RM -> XAUI/5gbase-r/2500base-x/SGMII 2 -> 0 RXAUI with RM -> RXAUI/5gbase-r/2500base-x/SGMII 6 -> 4 XFI with RM -> XFI/5gbase-r/2500base-x/SGMII I can put these thought into the commit message, if requested. > I'm afraid I haven't put much thought into how to solve it, and as I'm > totally demotivated at the moment, that's unlikely to change. I am sorry to hear that, Russell :-( Usually for me a lack of motivation is caused by bad mood (and also vice-versa, so this can result in a self-feeding loop). What I found that helps me with this is a good book to read. If you are open to suggestions, the (IMO) best book I ever read is Harry Potter and the Methods of Rationality (it's for free and online). Marek
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 1901ba277413..9e8e9aa66972 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -453,6 +453,33 @@ static bool mv3310_has_pma_ngbaset_quirk(struct phy_device *phydev) MV_PHY_ALASKA_NBT_QUIRK_MASK) == MV_PHY_ALASKA_NBT_QUIRK_REV; } +static int mv3310_select_mactype(struct phy_device *phydev) +{ + int mac_type, ret; + + /* On some devices the MAC does not support 10G mode, but may support + * lower modes, such as SGMII or 2500base-x. + * By changing MACTYPE of the PHY to 4 in this case, we ensure that + * the MAC will link with the PHY at least for these lower speeds. + */ + switch (phydev->interface) { + case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_2500BASEX: + mac_type = 4; + break; + default: + return 0; + } + + ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, + MV_V2_PORT_MAC_TYPE_MASK, mac_type); + if (ret <= 0) + return ret; + + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, + MV_V2_PORT_CTRL_SWRST, MV_V2_PORT_CTRL_SWRST); +} + static int mv3310_config_init(struct phy_device *phydev) { struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); @@ -474,6 +501,10 @@ static int mv3310_config_init(struct phy_device *phydev) if (err) return err; + err = mv3310_select_mactype(phydev); + if (err) + return err; + val = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL); if (val < 0) return val;
RollBall SFPs contain a Marvell 88X3310 PHY, but by default the MACTYPE is set to 10GBASE-R with Rate Matching. Some devices (for example those based on Armada 38x) only support up to 2500base-x SerDes modes. Change the PHY's MACTYPE to 4 (which means changing between 10gbase-r, 5gbase-r, 2500base-x ans SGMII depending on copper speed) if this is the case (which is infered from phydev->interface). Signed-off-by: Marek Behún <kabel@kernel.org> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/marvell10g.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)