The embedded PHYs of the MV88E6020 (88E6250 family) switch are very
basic - they do not even have an Extended Address / Page register.
This adds support for the PHYs to the driver to set up PHY interrupts
and retrieve error stats. The ethtool stat functions are slightly
refactored to allow dealing with different marvell_hw_stat definitions.
The same code should also work with other 88E6250 family switches
(6250/6220/6071/6070), but it is unclear whether these use the same PHY
ID - the spec only lists the prefix 0x01410c00 and describes the lower
10 bits as reserved, but that seems too unspecific to be useful, as it
would cover several existing PHY IDs already supported by the driver.
Signed-off-by: Matthias Schiffer <[email protected]>
---
I guess one option regarding the PHY IDs would be to use 0x01410c00 with
mask 0xfffffc00 (lower 10 bits unspecified to match the docs) as a
fallback that would cover various PHYs without more specific support,
instead of making it specific to the MV88E6020 - the receive error stat
and interrupt handling seem to be common amongst all models supported by
the driver. Opinions?
drivers/net/phy/marvell.c | 79 +++++++++++++++++++++++++++++++++----
include/linux/marvell_phy.h | 3 ++
2 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 860dc4001d415..7debaeca2620b 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -301,6 +301,7 @@
#define LPA_PAUSE_ASYM_FIBER 0x100
#define NB_FIBER_STATS 1
+#define NB_STAT_MAX 3
MODULE_DESCRIPTION("Marvell PHY driver");
MODULE_AUTHOR("Andy Fleming");
@@ -319,6 +320,14 @@ static struct marvell_hw_stat marvell_hw_stats[] = {
{ "phy_receive_errors_fiber", 1, 21, 16},
};
+static_assert(ARRAY_SIZE(marvell_hw_stats) <= NB_STAT_MAX);
+
+static struct marvell_hw_stat marvell_hw_stats_88e6020[] = {
+ { "phy_receive_errors", 0, 21, 16},
+};
+
+static_assert(ARRAY_SIZE(marvell_hw_stats_88e6020) <= NB_STAT_MAX);
+
enum {
M88E3082_VCT_OFF,
M88E3082_VCT_PHASE1,
@@ -326,7 +335,7 @@ enum {
};
struct marvell_priv {
- u64 stats[ARRAY_SIZE(marvell_hw_stats)];
+ u64 stats[NB_STAT_MAX];
char *hwmon_name;
struct device *hwmon_dev;
bool cable_test_tdr;
@@ -1978,25 +1987,53 @@ static int marvell_get_sset_count(struct phy_device *phydev)
return ARRAY_SIZE(marvell_hw_stats) - NB_FIBER_STATS;
}
-static void marvell_get_strings(struct phy_device *phydev, u8 *data)
+static int marvell_get_sset_count_88e6020(struct phy_device *phydev)
+{
+ return ARRAY_SIZE(marvell_hw_stats_88e6020);
+}
+
+static void marvell_copy_strings(struct phy_device *phydev, u8 *data,
+ const struct marvell_hw_stat *stats,
+ int count)
{
- int count = marvell_get_sset_count(phydev);
int i;
for (i = 0; i < count; i++) {
strscpy(data + i * ETH_GSTRING_LEN,
- marvell_hw_stats[i].string, ETH_GSTRING_LEN);
+ stats[i].string, ETH_GSTRING_LEN);
}
}
-static u64 marvell_get_stat(struct phy_device *phydev, int i)
+static void marvell_get_strings(struct phy_device *phydev, u8 *data)
+{
+ int count = marvell_get_sset_count(phydev);
+
+ marvell_copy_strings(phydev, data, marvell_hw_stats, count);
+}
+
+static void marvell_get_strings_88e6020(struct phy_device *phydev, u8 *data)
+{
+ int count = marvell_get_sset_count_88e6020(phydev);
+
+ marvell_copy_strings(phydev, data, marvell_hw_stats_88e6020, count);
+}
+
+static u64 marvell_get_stat(struct phy_device *phydev,
+ const struct marvell_hw_stat *stats,
+ int i)
{
- struct marvell_hw_stat stat = marvell_hw_stats[i];
+ struct marvell_hw_stat stat = stats[i];
struct marvell_priv *priv = phydev->priv;
int val;
u64 ret;
- val = phy_read_paged(phydev, stat.page, stat.reg);
+ if (phydev->drv->write_page)
+ val = phy_read_paged(phydev, stat.page, stat.reg);
+ else if (stat.page == 0)
+ val = phy_read(phydev, stat.reg);
+ else
+ val = -1;
+
if (val < 0) {
ret = U64_MAX;
} else {
@@ -2015,7 +2052,17 @@ static void marvell_get_stats(struct phy_device *phydev,
int i;
for (i = 0; i < count; i++)
- data[i] = marvell_get_stat(phydev, i);
+ data[i] = marvell_get_stat(phydev, marvell_hw_stats, i);
+}
+
+static void marvell_get_stats_88e6020(struct phy_device *phydev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ int count = marvell_get_sset_count_88e6020(phydev);
+ int i;
+
+ for (i = 0; i < count; i++)
+ data[i] = marvell_get_stat(phydev, marvell_hw_stats_88e6020, i);
}
static int m88e1510_loopback(struct phy_device *phydev, bool enable)
@@ -3924,6 +3971,21 @@ static struct phy_driver marvell_drivers[] = {
.get_strings = marvell_get_strings,
.get_stats = marvell_get_stats,
},
+ {
+ .phy_id = MARVELL_PHY_ID_88E6020,
+ .phy_id_mask = MARVELL_PHY_ID_MASK,
+ .name = "Marvell 88E6020",
+ /* PHY_BASIC_FEATURES */
+ .probe = marvell_probe,
+ .aneg_done = marvell_aneg_done,
+ .config_intr = marvell_config_intr,
+ .handle_interrupt = marvell_handle_interrupt,
+ .resume = genphy_resume,
+ .suspend = genphy_suspend,
+ .get_sset_count = marvell_get_sset_count_88e6020,
+ .get_strings = marvell_get_strings_88e6020,
+ .get_stats = marvell_get_stats_88e6020,
+ },
{
.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
.phy_id_mask = MARVELL_PHY_ID_MASK,
@@ -4072,6 +4134,7 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
+ { MARVELL_PHY_ID_88E6020, MARVELL_PHY_ID_MASK },
{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
{ MARVELL_PHY_ID_88E6393_FAMILY, MARVELL_PHY_ID_MASK },
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 88254f9aec2b2..44e31bd3e08be 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -32,6 +32,9 @@
/* Marvel 88E1111 in Finisar SFP module with modified PHY ID */
#define MARVELL_PHY_ID_88E1111_FINISAR 0x01ff0cc0
+/* Embedded switch PHY IDs */
+#define MARVELL_PHY_ID_88E6020 0x01410db0
+
/* These Ethernet switch families contain embedded PHYs, but they do
* not have a model ID. So the switch driver traps reads to the ID2
* register and returns the switch family ID
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
On Wed, 2024-04-24 at 14:42 +0200, Andrew Lunn wrote:
> On Wed, Apr 24, 2024 at 02:10:22PM +0200, Matthias Schiffer wrote:
> > The embedded PHYs of the MV88E6020 (88E6250 family) switch are very
> > basic - they do not even have an Extended Address / Page register.
> >
> > This adds support for the PHYs to the driver to set up PHY interrupts
> > and retrieve error stats. The ethtool stat functions are slightly
> > refactored to allow dealing with different marvell_hw_stat definitions.
> >
> > The same code should also work with other 88E6250 family switches
> > (6250/6220/6071/6070), but it is unclear whether these use the same PHY
> > ID - the spec only lists the prefix 0x01410c00 and describes the lower
> > 10 bits as reserved, but that seems too unspecific to be useful, as it
> > would cover several existing PHY IDs already supported by the driver.
>
> This sounds like:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/mv88e6xxx/chip.c#L3642
>
> So the DSA driver will fill in the lower bits, and it should work for
> all devices in the family.
>
> > @@ -4072,6 +4134,7 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
> > { MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
> > { MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
> > { MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
> > + { MARVELL_PHY_ID_88E6020, MARVELL_PHY_ID_MASK },
> > { MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
>
> Please follow the naming convention. MARVELL_PHY_ID_88E6250_FAMILY.
>
> Andrew
We currently do not override the PHY ID for this family in the DSA driver - my understanding was
that this is only necessary for switches that don't provide a usable PHY ID at all. As I have no
idea if this PHY ID works for the whole family or just the single switch, I went with the more
specific naming here.
Matthias
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
On Wed, 2024-04-24 at 16:52 +0200, Andrew Lunn wrote:
> > We currently do not override the PHY ID for this family in the DSA
> > driver - my understanding was that this is only necessary for
> > switches that don't provide a usable PHY ID at all. As I have no
> > idea if this PHY ID works for the whole family or just the single
> > switch, I went with the more specific naming here.
>
> The 'broken' switches have the Marvell OUI, but no module number. From
> your description it sounds like the 6250 is the same?
>
No, the PHYs have a proper ID, 0x01410db0. The "Functional Spec" for the 88E6020 switch family gives
us the 0x01410c00 part, but the lower 10 bits are undocumented ("reserved"), and I don't know if
they differ for each individual switch of the family, as I only have a 88E6020 here.
Matthias
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
On Wed, Apr 24, 2024 at 04:57:24PM +0200, Matthias Schiffer wrote:
> On Wed, 2024-04-24 at 16:52 +0200, Andrew Lunn wrote:
> > > We currently do not override the PHY ID for this family in the DSA
> > > driver - my understanding was that this is only necessary for
> > > switches that don't provide a usable PHY ID at all. As I have no
> > > idea if this PHY ID works for the whole family or just the single
> > > switch, I went with the more specific naming here.
> >
> > The 'broken' switches have the Marvell OUI, but no module number. From
> > your description it sounds like the 6250 is the same?
> >
>
> No, the PHYs have a proper ID, 0x01410db0. The "Functional Spec" for the 88E6020 switch family gives
> us the 0x01410c00 part, but the lower 10 bits are undocumented ("reserved")
Great, Marvell breaking things in new ways :-(
It would be so much nicer if they broke things consistently.
> and I don't know if they differ for each individual switch of the
> family, as I only have a 88E6020 here.
Marvell are pretty consistent within a family. So i expect this same
ID is used for them all. However, i would use MARVELL_PHY_ID_MASK so
the lower nibble is ignored.
Please document these assumptions in the commit message.
Andrew
On Wed, Apr 24, 2024 at 02:10:22PM +0200, Matthias Schiffer wrote:
> The embedded PHYs of the MV88E6020 (88E6250 family) switch are very
> basic - they do not even have an Extended Address / Page register.
>
> This adds support for the PHYs to the driver to set up PHY interrupts
> and retrieve error stats. The ethtool stat functions are slightly
> refactored to allow dealing with different marvell_hw_stat definitions.
>
> The same code should also work with other 88E6250 family switches
> (6250/6220/6071/6070), but it is unclear whether these use the same PHY
> ID - the spec only lists the prefix 0x01410c00 and describes the lower
> 10 bits as reserved, but that seems too unspecific to be useful, as it
> would cover several existing PHY IDs already supported by the driver.
This sounds like:
https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/mv88e6xxx/chip.c#L3642
So the DSA driver will fill in the lower bits, and it should work for
all devices in the family.
> @@ -4072,6 +4134,7 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
> { MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
> { MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
> { MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
> + { MARVELL_PHY_ID_88E6020, MARVELL_PHY_ID_MASK },
> { MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
Please follow the naming convention. MARVELL_PHY_ID_88E6250_FAMILY.
Andrew
> We currently do not override the PHY ID for this family in the DSA
> driver - my understanding was that this is only necessary for
> switches that don't provide a usable PHY ID at all. As I have no
> idea if this PHY ID works for the whole family or just the single
> switch, I went with the more specific naming here.
The 'broken' switches have the Marvell OUI, but no module number. From
your description it sounds like the 6250 is the same?
Andrew
On Wed, Apr 24, 2024 at 02:10:22PM +0200, Matthias Schiffer wrote:
> The embedded PHYs of the MV88E6020 (88E6250 family) switch are very
> basic - they do not even have an Extended Address / Page register.
> +static struct marvell_hw_stat marvell_hw_stats_88e6020[] = {
> + { "phy_receive_errors", 0, 21, 16},
So the 0 here is bogus?
This patch is quite invasive, and it is hard to say it is worth it. I
would probably not try to reuses as much code, add functions which are
specific to the 6250. Keep it KISS.
Andrew