| Message ID | 20210325131250.15901-12-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell10g updates | 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/cc_maintainers | warning | 1 maintainers not CCed: linux@armlinux.org.uk |
| 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, 57 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, 25 Mar 2021 14:12:49 +0100
Marek Behún <kabel@kernel.org> wrote:
> Print exact mode, one of
typo: model
On Thu, Mar 25, 2021 at 02:12:49PM +0100, Marek Behún wrote: > @@ -443,12 +446,24 @@ static int mv3310_probe(struct phy_device *phydev) > > switch (phydev->drv->phy_id) { > case MARVELL_PHY_ID_88X3310: > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_XGSTAT); > + if (ret < 0) > + return ret; > + > + has_macsec = !(ret & MV_PMA_XGSTAT_NO_MACSEC); > + > if (nports == 4) > priv->model = MV_MODEL_88X3340; > else if (nports == 1) > priv->model = MV_MODEL_88X3310; > break; The 88X3310 and 88X3340 can be differentiated by bit 3 in the revision. In other words, 88X3310 is 0x09a0..0x09a7, and 88X3340 is 0x09a8..0x09af. We could add a separate driver structure, which would then allow the kernel to print a more specific string via standard methods, like we do for other PHYs. Not sure whether that would work for the 88X21x0 family though.
On Thu, 25 Mar 2021 15:54:52 +0000 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Thu, Mar 25, 2021 at 02:12:49PM +0100, Marek Behún wrote: > > @@ -443,12 +446,24 @@ static int mv3310_probe(struct phy_device *phydev) > > > > switch (phydev->drv->phy_id) { > > case MARVELL_PHY_ID_88X3310: > > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_XGSTAT); > > + if (ret < 0) > > + return ret; > > + > > + has_macsec = !(ret & MV_PMA_XGSTAT_NO_MACSEC); > > + > > if (nports == 4) > > priv->model = MV_MODEL_88X3340; > > else if (nports == 1) > > priv->model = MV_MODEL_88X3310; > > break; > > The 88X3310 and 88X3340 can be differentiated by bit 3 in the revision. > In other words, 88X3310 is 0x09a0..0x09a7, and 88X3340 is > 0x09a8..0x09af. We could add a separate driver structure, which would > then allow the kernel to print a more specific string via standard > methods, like we do for other PHYs. Not sure whether that would work > for the 88X21x0 family though. OK I will look into this. What are your thoughts on the other patches? Marek
On Thu, 25 Mar 2021 15:54:52 +0000 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > The 88X3310 and 88X3340 can be differentiated by bit 3 in the revision. > In other words, 88X3310 is 0x09a0..0x09a7, and 88X3340 is > 0x09a8..0x09af. We could add a separate driver structure, which would > then allow the kernel to print a more specific string via standard > methods, like we do for other PHYs. Not sure whether that would work > for the 88X21x0 family though. According to release notes it seems that we can also differentiate 88E211X from 88E218X (via bit 3 in register 1.3): 88E211X has 0x09B9 88E218X has 0x09B1 but not 88E2110 from 88E2111 nor 88E2180 from 88E2181. These can be differentiated via register 3.0004.7 (bit 7 of MDIO_MMD_PCS.MDIO_SPEED., which says whether device is capable of 5g speed) I propose creating separate structures for mv88x3340 and mv88e218x. We can then print the remaining info as "(not) macsec/ptp capable" or "(not) 5g capable" What do you think? Marek
On 25.03.2021 21:29, Marek Behún wrote: > On Thu, 25 Mar 2021 15:54:52 +0000 > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > >> The 88X3310 and 88X3340 can be differentiated by bit 3 in the revision. >> In other words, 88X3310 is 0x09a0..0x09a7, and 88X3340 is >> 0x09a8..0x09af. We could add a separate driver structure, which would >> then allow the kernel to print a more specific string via standard >> methods, like we do for other PHYs. Not sure whether that would work >> for the 88X21x0 family though. > > According to release notes it seems that we can also differentiate > 88E211X from 88E218X (via bit 3 in register 1.3): > 88E211X has 0x09B9 > 88E218X has 0x09B1 > > but not 88E2110 from 88E2111 > nor 88E2180 from 88E2181. > > These can be differentiated via register > 3.0004.7 > (bit 7 of MDIO_MMD_PCS.MDIO_SPEED., which says whether device is capable > of 5g speed) > If the PHY ID's are the same but you can use this register to differentiate the two versions, then you could implement the match_phy_device callback. This would allow you to have separate PHY drivers. This is just meant to say you have this option, I don't know the context good enough to state whether it's the better one. > I propose creating separate structures for mv88x3340 and mv88e218x. > We can then print the remaining info as > "(not) macsec/ptp capable" > or > "(not) 5g capable" > > What do you think? > > Marek >
On Thu, 25 Mar 2021 21:44:21 +0100 Heiner Kallweit <hkallweit1@gmail.com> wrote: > On 25.03.2021 21:29, Marek Behún wrote: > > On Thu, 25 Mar 2021 15:54:52 +0000 > > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > >> The 88X3310 and 88X3340 can be differentiated by bit 3 in the revision. > >> In other words, 88X3310 is 0x09a0..0x09a7, and 88X3340 is > >> 0x09a8..0x09af. We could add a separate driver structure, which would > >> then allow the kernel to print a more specific string via standard > >> methods, like we do for other PHYs. Not sure whether that would work > >> for the 88X21x0 family though. > > > > According to release notes it seems that we can also differentiate > > 88E211X from 88E218X (via bit 3 in register 1.3): > > 88E211X has 0x09B9 > > 88E218X has 0x09B1 > > > > but not 88E2110 from 88E2111 > > nor 88E2180 from 88E2181. > > > > These can be differentiated via register > > 3.0004.7 > > (bit 7 of MDIO_MMD_PCS.MDIO_SPEED., which says whether device is capable > > of 5g speed) > > > > If the PHY ID's are the same but you can use this register to > differentiate the two versions, then you could implement the > match_phy_device callback. This would allow you to have separate > PHY drivers. This is just meant to say you have this option, I don't > know the context good enough to state whether it's the better one. Nice, didn't know about that. But I fear whether this would always work for the 88X3310 vs 88X3310P, it is possible that this feature is only recognizable if the firmware in the PHY is already running. I shall look into this. Marek
On Thu, Mar 25, 2021 at 09:54:14PM +0100, Marek Behún wrote: > On Thu, 25 Mar 2021 21:44:21 +0100 > Heiner Kallweit <hkallweit1@gmail.com> wrote: > > > On 25.03.2021 21:29, Marek Behún wrote: > > > On Thu, 25 Mar 2021 15:54:52 +0000 > > > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > > > >> The 88X3310 and 88X3340 can be differentiated by bit 3 in the revision. > > >> In other words, 88X3310 is 0x09a0..0x09a7, and 88X3340 is > > >> 0x09a8..0x09af. We could add a separate driver structure, which would > > >> then allow the kernel to print a more specific string via standard > > >> methods, like we do for other PHYs. Not sure whether that would work > > >> for the 88X21x0 family though. > > > > > > According to release notes it seems that we can also differentiate > > > 88E211X from 88E218X (via bit 3 in register 1.3): > > > 88E211X has 0x09B9 > > > 88E218X has 0x09B1 > > > > > > but not 88E2110 from 88E2111 > > > nor 88E2180 from 88E2181. > > > > > > These can be differentiated via register > > > 3.0004.7 > > > (bit 7 of MDIO_MMD_PCS.MDIO_SPEED., which says whether device is capable > > > of 5g speed) > > > > > > > If the PHY ID's are the same but you can use this register to > > differentiate the two versions, then you could implement the > > match_phy_device callback. This would allow you to have separate > > PHY drivers. This is just meant to say you have this option, I don't > > know the context good enough to state whether it's the better one. > > Nice, didn't know about that. But I fear whether this would always work > for the 88X3310 vs 88X3310P, it is possible that this feature is only > recognizable if the firmware in the PHY is already running. The ID registers aren't programmable and contain the proper IDs even if there isn't firmware loaded (I've had such a PHY here.)
On Fri, 26 Mar 2021 09:07:34 +0000 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > Nice, didn't know about that. But I fear whether this would always work > > for the 88X3310 vs 88X3310P, it is possible that this feature is only > > recognizable if the firmware in the PHY is already running. > > The ID registers aren't programmable and contain the proper IDs even if > there isn't firmware loaded (I've had such a PHY here.) > Yes, but the macsec feature bit is in register MDIO_MMD_PMAPMD.MV_PMA_XGSTAT.12 (1.c001.12) But it says "This bit is valid upon completion of reset (1.0.15 = 0)", so it seems we can use this. :) Marek
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 6df67c12f012..84f24fcb832c 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -33,6 +33,8 @@ #define MV_PHY_ALASKA_NBT_QUIRK_REV (MARVELL_PHY_ID_88X3310 | 0xa) enum { + MV_PMA_XGSTAT = 0xc001, + MV_PMA_XGSTAT_NO_MACSEC = BIT(12), MV_PMA_FW_VER0 = 0xc011, MV_PMA_FW_VER1 = 0xc012, MV_PMA_21X0_PORT_CTRL = 0xc04a, @@ -397,6 +399,7 @@ static int mv3310_probe(struct phy_device *phydev) { struct mv3310_priv *priv; u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN; + bool has_5g, has_macsec; int ret, nports; if (!phydev->is_c45 || @@ -443,12 +446,24 @@ static int mv3310_probe(struct phy_device *phydev) switch (phydev->drv->phy_id) { case MARVELL_PHY_ID_88X3310: + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_XGSTAT); + if (ret < 0) + return ret; + + has_macsec = !(ret & MV_PMA_XGSTAT_NO_MACSEC); + if (nports == 4) priv->model = MV_MODEL_88X3340; else if (nports == 1) priv->model = MV_MODEL_88X3310; break; case MARVELL_PHY_ID_88E2110: + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_SPEED); + if (ret < 0) + return ret; + + has_5g = ret & MDIO_PCS_SPEED_5G; + if (nports == 8) priv->model = MV_MODEL_88E218X; else if (nports == 1) @@ -458,7 +473,17 @@ static int mv3310_probe(struct phy_device *phydev) unreachable(); } - if (!priv->model) { + switch (priv->model) { + case MV_MODEL_88E211X: + case MV_MODEL_88E218X: + phydev_info(phydev, "model 88E21%d%d\n", nports, !has_5g); + break; + case MV_MODEL_88X3310: + case MV_MODEL_88X3340: + phydev_info(phydev, "model 88X33%d0%s\n", nports, + has_macsec ? "P" : ""); + break; + default: phydev_err(phydev, "unknown PHY model (nports = %i)\n", nports); return -ENODEV; }
Print exact mode, one of 88E2110 88E2111 88E2180 88E2181 88X3310 88X3310P 88X3340 88X3340P Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/net/phy/marvell10g.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)