| Message ID | 20210412121430.20898-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell: fix detection of PHY on Topaz switches | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
> +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family); > + No forward declaration please. Move the code around. It is often best to do that in a patch which just moves code, no other changes. It makes it easier to review. > static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) > { > struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv; > @@ -3040,24 +3042,9 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) > err = chip->info->ops->phy_read(chip, bus, phy, reg, &val); > mv88e6xxx_reg_unlock(chip); > > - if (reg == MII_PHYSID2) { > - /* Some internal PHYs don't have a model number. */ > - if (chip->info->family != MV88E6XXX_FAMILY_6165) > - /* Then there is the 6165 family. It gets is > - * PHYs correct. But it can also have two > - * SERDES interfaces in the PHY address > - * space. And these don't have a model > - * number. But they are not PHYs, so we don't > - * want to give them something a PHY driver > - * will recognise. > - * > - * Use the mv88e6390 family model number > - * instead, for anything which really could be > - * a PHY, > - */ > - if (!(val & 0x3f0)) > - val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; > - } > + /* Some internal PHYs don't have a model number. */ > + if (reg == MII_PHYSID2 && !(val & 0x3f0)) > + val |= mv88e6xxx_physid_for_family(chip->info->family); > > return err ? err : val; > } > @@ -5244,6 +5231,39 @@ static const struct mv88e6xxx_info *mv88e6xxx_lookup_info(unsigned int prod_num) > return NULL; > } > > +/* This table contains representative model for every family */ > +static const enum mv88e6xxx_model family_model_table[] = { > + [MV88E6XXX_FAMILY_6095] = MV88E6095, > + [MV88E6XXX_FAMILY_6097] = MV88E6097, > + [MV88E6XXX_FAMILY_6185] = MV88E6185, > + [MV88E6XXX_FAMILY_6250] = MV88E6250, > + [MV88E6XXX_FAMILY_6320] = MV88E6320, > + [MV88E6XXX_FAMILY_6341] = MV88E6341, > + [MV88E6XXX_FAMILY_6351] = MV88E6351, > + [MV88E6XXX_FAMILY_6352] = MV88E6352, > + [MV88E6XXX_FAMILY_6390] = MV88E6390, > +}; This table is wrong. MV88E6390 does not equal MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390 was chosen because it is already an MDIO device ID, in register 2 and 3. It probably will never clash with a real Marvell PHY ID. MV88E6390 is just a small integer, and there is a danger it will clash with a real PHY. > --- a/drivers/net/phy/marvell.c > +++ b/drivers/net/phy/marvell.c > @@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = { > .get_stats = marvell_get_stats, > }, > { > - .phy_id = MARVELL_PHY_ID_88E6390, > + .phy_id = MARVELL_PHY_ID_88E6341_FAMILY, > .phy_id_mask = MARVELL_PHY_ID_MASK, > - .name = "Marvell 88E6390", > + .name = "Marvell 88E6341 Family", You cannot just replace the MARVELL_PHY_ID_88E6390. That will break the 6390! You need to add the new PHY for the 88E6341. Andrew
On Monday 12 April 2021 15:15:07 Andrew Lunn wrote: > > +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family); > > + > > No forward declaration please. Move the code around. It is often best > to do that in a patch which just moves code, no other changes. It > makes it easier to review. Avoiding forward declaration would mean to move about half of source code. mv88e6xxx_physid_for_family depends on mv88e6xxx_table which depends on all _ops structures which depends on all lot of other functions. I wanted to create a small fixup patch which can be easily backported to stable releases which are affected by this issue. If you do not like forward declarations, I can create a followup patch which moves this half of code in this file to avoid forward declaration. But having it in one patch would mean to complicate reviewing code... > > static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) > > { > > struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv; > > @@ -3040,24 +3042,9 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) > > err = chip->info->ops->phy_read(chip, bus, phy, reg, &val); > > mv88e6xxx_reg_unlock(chip); > > > > - if (reg == MII_PHYSID2) { > > - /* Some internal PHYs don't have a model number. */ > > - if (chip->info->family != MV88E6XXX_FAMILY_6165) > > - /* Then there is the 6165 family. It gets is > > - * PHYs correct. But it can also have two > > - * SERDES interfaces in the PHY address > > - * space. And these don't have a model > > - * number. But they are not PHYs, so we don't > > - * want to give them something a PHY driver > > - * will recognise. > > - * > > - * Use the mv88e6390 family model number > > - * instead, for anything which really could be > > - * a PHY, > > - */ > > - if (!(val & 0x3f0)) > > - val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; > > - } > > + /* Some internal PHYs don't have a model number. */ > > + if (reg == MII_PHYSID2 && !(val & 0x3f0)) > > + val |= mv88e6xxx_physid_for_family(chip->info->family); > > > > return err ? err : val; > > } > > @@ -5244,6 +5231,39 @@ static const struct mv88e6xxx_info *mv88e6xxx_lookup_info(unsigned int prod_num) > > return NULL; > > } > > > > +/* This table contains representative model for every family */ > > +static const enum mv88e6xxx_model family_model_table[] = { > > + [MV88E6XXX_FAMILY_6095] = MV88E6095, > > + [MV88E6XXX_FAMILY_6097] = MV88E6097, > > + [MV88E6XXX_FAMILY_6185] = MV88E6185, > > + [MV88E6XXX_FAMILY_6250] = MV88E6250, > > + [MV88E6XXX_FAMILY_6320] = MV88E6320, > > + [MV88E6XXX_FAMILY_6341] = MV88E6341, > > + [MV88E6XXX_FAMILY_6351] = MV88E6351, > > + [MV88E6XXX_FAMILY_6352] = MV88E6352, > > + [MV88E6XXX_FAMILY_6390] = MV88E6390, > > +}; > > This table is wrong. MV88E6390 does not equal > MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390 > was chosen because it is already an MDIO device ID, in register 2 and > 3. It probably will never clash with a real Marvell PHY ID. MV88E6390 > is just a small integer, and there is a danger it will clash with a > real PHY. So... how to solve this issue? What should be in the mapping table? > > --- a/drivers/net/phy/marvell.c > > +++ b/drivers/net/phy/marvell.c > > @@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = { > > .get_stats = marvell_get_stats, > > }, > > { > > - .phy_id = MARVELL_PHY_ID_88E6390, > > + .phy_id = MARVELL_PHY_ID_88E6341_FAMILY, > > .phy_id_mask = MARVELL_PHY_ID_MASK, > > - .name = "Marvell 88E6390", > > + .name = "Marvell 88E6341 Family", > > You cannot just replace the MARVELL_PHY_ID_88E6390. That will break > the 6390! You need to add the new PHY for the 88E6341. I have not replaced anything. I just put there a new phy_id section for 88E6341. You are probably confused by 'git diff' output as quickly looking at it, somebody can think that there is phy replacement. But there is no replacement, I only added a new PHY. Entry for 88E6390 is still there! Also this is reason why I wanted to avoid big code movement. It will be harder to read the 'git diff' output in this patch.
> > > +/* This table contains representative model for every family */ > > > +static const enum mv88e6xxx_model family_model_table[] = { > > > + [MV88E6XXX_FAMILY_6095] = MV88E6095, > > > + [MV88E6XXX_FAMILY_6097] = MV88E6097, > > > + [MV88E6XXX_FAMILY_6185] = MV88E6185, > > > + [MV88E6XXX_FAMILY_6250] = MV88E6250, > > > + [MV88E6XXX_FAMILY_6320] = MV88E6320, > > > + [MV88E6XXX_FAMILY_6341] = MV88E6341, > > > + [MV88E6XXX_FAMILY_6351] = MV88E6351, > > > + [MV88E6XXX_FAMILY_6352] = MV88E6352, > > > + [MV88E6XXX_FAMILY_6390] = MV88E6390, > > > +}; > > > > This table is wrong. MV88E6390 does not equal > > MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390 > > was chosen because it is already an MDIO device ID, in register 2 and > > 3. It probably will never clash with a real Marvell PHY ID. MV88E6390 > > is just a small integer, and there is a danger it will clash with a > > real PHY. > > So... how to solve this issue? What should be in the mapping table? You need to use MV88E6XXX_PORT_SWITCH_ID_PROD_6095, MV88E6XXX_PORT_SWITCH_ID_PROD_6097, ... MV88E6XXX_PORT_SWITCH_ID_PROD_6390, > > You cannot just replace the MARVELL_PHY_ID_88E6390. That will break > > the 6390! You need to add the new PHY for the 88E6341. > > I have not replaced anything. Yes, sorry. I read the diff wrong. Andrew
On Monday 12 April 2021 16:30:03 Andrew Lunn wrote: > > > > +/* This table contains representative model for every family */ > > > > +static const enum mv88e6xxx_model family_model_table[] = { > > > > + [MV88E6XXX_FAMILY_6095] = MV88E6095, > > > > + [MV88E6XXX_FAMILY_6097] = MV88E6097, > > > > + [MV88E6XXX_FAMILY_6185] = MV88E6185, > > > > + [MV88E6XXX_FAMILY_6250] = MV88E6250, > > > > + [MV88E6XXX_FAMILY_6320] = MV88E6320, > > > > + [MV88E6XXX_FAMILY_6341] = MV88E6341, > > > > + [MV88E6XXX_FAMILY_6351] = MV88E6351, > > > > + [MV88E6XXX_FAMILY_6352] = MV88E6352, > > > > + [MV88E6XXX_FAMILY_6390] = MV88E6390, > > > > +}; > > > > > > This table is wrong. MV88E6390 does not equal > > > MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390 > > > was chosen because it is already an MDIO device ID, in register 2 and > > > 3. It probably will never clash with a real Marvell PHY ID. MV88E6390 > > > is just a small integer, and there is a danger it will clash with a > > > real PHY. > > > > So... how to solve this issue? What should be in the mapping table? > > You need to use MV88E6XXX_PORT_SWITCH_ID_PROD_6095, > MV88E6XXX_PORT_SWITCH_ID_PROD_6097, > ... > MV88E6XXX_PORT_SWITCH_ID_PROD_6390, But I'm using it. First chip->info->family (enum mv88e6xxx_family; MV88E6XXX_FAMILY_6341) is mapped to enum mv88e6xxx_model (MV88E6341) via family_model_table[] and then enum mv88e6xxx_model (MV88E6341) is mapped to prod_num (MV88E6XXX_PORT_SWITCH_ID_PROD_6341) via mv88e6xxx_table[]. All this is done in mv88e6xxx_physid_for_family() function. So at the end, this function converts MV88E6XXX_FAMILY_6341 to MV88E6XXX_PORT_SWITCH_ID_PROD_6341. And therefore I do not see anything wrong in family_model_table[] table. I defined family_model_table[] table to just maps enum mv88e6xxx_family to enum mv88e6xxx_model as mv88e6xxx_table[] table already contains mapping from enum mv88e6xxx_model to phys_id, to simplify implementation.
On Mon, Apr 12, 2021 at 03:34:47PM +0200, Pali Rohár wrote: > On Monday 12 April 2021 15:15:07 Andrew Lunn wrote: > > > +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family); > > > + > > > > No forward declaration please. Move the code around. It is often best > > to do that in a patch which just moves code, no other changes. It > > makes it easier to review. > > Avoiding forward declaration would mean to move about half of source > code. mv88e6xxx_physid_for_family depends on mv88e6xxx_table which > depends on all _ops structures which depends on all lot of other > functions. So this is basically what you are trying to do: diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 903d619e08ed..ef4dbcb052b7 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3026,6 +3026,18 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) return err; } +static const enum mv88e6xxx_model family_model_table[] = { + [MV88E6XXX_FAMILY_6095] = MV88E6XXX_PORT_SWITCH_ID_PROD_6095, + [MV88E6XXX_FAMILY_6097] = MV88E6XXX_PORT_SWITCH_ID_PROD_6097, + [MV88E6XXX_FAMILY_6185] = MV88E6XXX_PORT_SWITCH_ID_PROD_6185, + [MV88E6XXX_FAMILY_6250] = MV88E6XXX_PORT_SWITCH_ID_PROD_6250, + [MV88E6XXX_FAMILY_6320] = MV88E6XXX_PORT_SWITCH_ID_PROD_6320, + [MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341, + [MV88E6XXX_FAMILY_6351] = MV88E6XXX_PORT_SWITCH_ID_PROD_6351, + [MV88E6XXX_FAMILY_6352] = MV88E6XXX_PORT_SWITCH_ID_PROD_6352, + [MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390, +}; + static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) { struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv; @@ -3056,7 +3068,7 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) * a PHY, */ if (!(val & 0x3f0)) - val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; + val |= family_model_table[chip->info->family] >> 4; } and it compiles. No forward declarations needed. It is missing all the error checking etc, but i don't see why that should change the dependencies. Andrew
On Monday 12 April 2021 16:44:11 Andrew Lunn wrote: > On Mon, Apr 12, 2021 at 03:34:47PM +0200, Pali Rohár wrote: > > On Monday 12 April 2021 15:15:07 Andrew Lunn wrote: > > > > +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family); > > > > + > > > > > > No forward declaration please. Move the code around. It is often best > > > to do that in a patch which just moves code, no other changes. It > > > makes it easier to review. > > > > Avoiding forward declaration would mean to move about half of source > > code. mv88e6xxx_physid_for_family depends on mv88e6xxx_table which > > depends on all _ops structures which depends on all lot of other > > functions. > > So this is basically what you are trying to do: > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 903d619e08ed..ef4dbcb052b7 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -3026,6 +3026,18 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) > return err; > } > > +static const enum mv88e6xxx_model family_model_table[] = { > + [MV88E6XXX_FAMILY_6095] = MV88E6XXX_PORT_SWITCH_ID_PROD_6095, > + [MV88E6XXX_FAMILY_6097] = MV88E6XXX_PORT_SWITCH_ID_PROD_6097, > + [MV88E6XXX_FAMILY_6185] = MV88E6XXX_PORT_SWITCH_ID_PROD_6185, > + [MV88E6XXX_FAMILY_6250] = MV88E6XXX_PORT_SWITCH_ID_PROD_6250, > + [MV88E6XXX_FAMILY_6320] = MV88E6XXX_PORT_SWITCH_ID_PROD_6320, > + [MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341, > + [MV88E6XXX_FAMILY_6351] = MV88E6XXX_PORT_SWITCH_ID_PROD_6351, > + [MV88E6XXX_FAMILY_6352] = MV88E6XXX_PORT_SWITCH_ID_PROD_6352, > + [MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390, > +}; Ok, no problem. I can change it in this way. I just thought that if prod_id is already defined for every model in mv88e6xxx_table[] table I could reuse it, instead of duplicating it... Anyway, now I'm looking at phy/marvell.c driver again and it supports only 88E6341 and 88E6390 families from whole 88E63xxx range. So do we need to define for now table for more than MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries? > + > static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) > { > struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv; > @@ -3056,7 +3068,7 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) > * a PHY, > */ > if (!(val & 0x3f0)) > - val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; > + val |= family_model_table[chip->info->family] >> 4; > } > > and it compiles. No forward declarations needed. It is missing all the > error checking etc, but i don't see why that should change the > dependencies. > > Andrew
> Anyway, now I'm looking at phy/marvell.c driver again and it supports > only 88E6341 and 88E6390 families from whole 88E63xxx range. > > So do we need to define for now table for more than > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries? Probably not. I've no idea if the 6393 has an ID, so to be safe you should add that. Assuming it has a family of its own. Andrew
On Monday 12 April 2021 17:32:33 Andrew Lunn wrote: > > Anyway, now I'm looking at phy/marvell.c driver again and it supports > > only 88E6341 and 88E6390 families from whole 88E63xxx range. > > > > So do we need to define for now table for more than > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries? > > Probably not. I've no idea if the 6393 has an ID, so to be safe you > should add that. Assuming it has a family of its own. So what about just? if (reg == MII_PHYSID2 && !(val & 0x3f0)) { if (chip->info->family == MV88E6XXX_FAMILY_6341) val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4; else if (chip->info->family == MV88E6XXX_FAMILY_6390) val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; }
On Mon, Apr 12, 2021 at 05:52:39PM +0200, Pali Rohár wrote: > On Monday 12 April 2021 17:32:33 Andrew Lunn wrote: > > > Anyway, now I'm looking at phy/marvell.c driver again and it supports > > > only 88E6341 and 88E6390 families from whole 88E63xxx range. > > > > > > So do we need to define for now table for more than > > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries? > > > > Probably not. I've no idea if the 6393 has an ID, so to be safe you > > should add that. Assuming it has a family of its own. > > So what about just? > > if (reg == MII_PHYSID2 && !(val & 0x3f0)) { > if (chip->info->family == MV88E6XXX_FAMILY_6341) > val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4; > else if (chip->info->family == MV88E6XXX_FAMILY_6390) > val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; > } As i said, i expect the 6393 also has no ID. And i recently found out Marvell have some automotive switches, 88Q5xxx which are actually based around the same IP and could be added to this driver. They also might not have an ID. I suspect this list is going to get longer, so having it table driven will make that simpler, less error prone. Andrew
On Monday 12 April 2021 18:12:35 Andrew Lunn wrote: > On Mon, Apr 12, 2021 at 05:52:39PM +0200, Pali Rohár wrote: > > On Monday 12 April 2021 17:32:33 Andrew Lunn wrote: > > > > Anyway, now I'm looking at phy/marvell.c driver again and it supports > > > > only 88E6341 and 88E6390 families from whole 88E63xxx range. > > > > > > > > So do we need to define for now table for more than > > > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries? > > > > > > Probably not. I've no idea if the 6393 has an ID, so to be safe you > > > should add that. Assuming it has a family of its own. > > > > So what about just? > > > > if (reg == MII_PHYSID2 && !(val & 0x3f0)) { > > if (chip->info->family == MV88E6XXX_FAMILY_6341) > > val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4; > > else if (chip->info->family == MV88E6XXX_FAMILY_6390) > > val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; > > } > > As i said, i expect the 6393 also has no ID. And i recently found out > Marvell have some automotive switches, 88Q5xxx which are actually > based around the same IP and could be added to this driver. They also > might not have an ID. I suspect this list is going to get longer, so > having it table driven will make that simpler, less error prone. > > Andrew Ok, I will use table but I fill it only with Topaz (6341) and Peridot (6390) which was there before as I do not have 6393 switch for testing. If you or anybody else has 6393 unit for testing, please extend then table.
On Mon, 12 Apr 2021 18:38:29 +0200 Pali Rohár <pali@kernel.org> wrote: > On Monday 12 April 2021 18:12:35 Andrew Lunn wrote: > > On Mon, Apr 12, 2021 at 05:52:39PM +0200, Pali Rohár wrote: > > > On Monday 12 April 2021 17:32:33 Andrew Lunn wrote: > > > > > Anyway, now I'm looking at phy/marvell.c driver again and it supports > > > > > only 88E6341 and 88E6390 families from whole 88E63xxx range. > > > > > > > > > > So do we need to define for now table for more than > > > > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries? > > > > > > > > Probably not. I've no idea if the 6393 has an ID, so to be safe you > > > > should add that. Assuming it has a family of its own. > > > > > > So what about just? > > > > > > if (reg == MII_PHYSID2 && !(val & 0x3f0)) { > > > if (chip->info->family == MV88E6XXX_FAMILY_6341) > > > val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4; > > > else if (chip->info->family == MV88E6XXX_FAMILY_6390) > > > val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; > > > } > > > > As i said, i expect the 6393 also has no ID. And i recently found out > > Marvell have some automotive switches, 88Q5xxx which are actually > > based around the same IP and could be added to this driver. They also > > might not have an ID. I suspect this list is going to get longer, so > > having it table driven will make that simpler, less error prone. > > > > Andrew > > Ok, I will use table but I fill it only with Topaz (6341) and Peridot > (6390) which was there before as I do not have 6393 switch for testing. > > If you or anybody else has 6393 unit for testing, please extend then > table. 6393 PHYs report PHY ID 0x002b0808, I.e. no model number. I now realize that I did not implement this for 6393, these PHYs are detected as mv88e6085 ... PHY [...] driver [Generic PHY] (irq=POLL) And it seems that this temperature sensor is different from 1510, 6341 and 6390 :) I will look into this and send a patch. Marek
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 903d619e08ed..e602c9816aee 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3026,6 +3026,8 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) return err; } +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family); + static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) { struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv; @@ -3040,24 +3042,9 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) err = chip->info->ops->phy_read(chip, bus, phy, reg, &val); mv88e6xxx_reg_unlock(chip); - if (reg == MII_PHYSID2) { - /* Some internal PHYs don't have a model number. */ - if (chip->info->family != MV88E6XXX_FAMILY_6165) - /* Then there is the 6165 family. It gets is - * PHYs correct. But it can also have two - * SERDES interfaces in the PHY address - * space. And these don't have a model - * number. But they are not PHYs, so we don't - * want to give them something a PHY driver - * will recognise. - * - * Use the mv88e6390 family model number - * instead, for anything which really could be - * a PHY, - */ - if (!(val & 0x3f0)) - val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4; - } + /* Some internal PHYs don't have a model number. */ + if (reg == MII_PHYSID2 && !(val & 0x3f0)) + val |= mv88e6xxx_physid_for_family(chip->info->family); return err ? err : val; } @@ -5244,6 +5231,39 @@ static const struct mv88e6xxx_info *mv88e6xxx_lookup_info(unsigned int prod_num) return NULL; } +/* This table contains representative model for every family */ +static const enum mv88e6xxx_model family_model_table[] = { + [MV88E6XXX_FAMILY_6095] = MV88E6095, + [MV88E6XXX_FAMILY_6097] = MV88E6097, + [MV88E6XXX_FAMILY_6185] = MV88E6185, + [MV88E6XXX_FAMILY_6250] = MV88E6250, + [MV88E6XXX_FAMILY_6320] = MV88E6320, + [MV88E6XXX_FAMILY_6341] = MV88E6341, + [MV88E6XXX_FAMILY_6351] = MV88E6351, + [MV88E6XXX_FAMILY_6352] = MV88E6352, + [MV88E6XXX_FAMILY_6390] = MV88E6390, +}; +static_assert(ARRAY_SIZE(family_model_table) == MV88E6XXX_FAMILY_LAST); + +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family) +{ + enum mv88e6xxx_model model; + + /* 6165 family gets it's PHYs correct. But it can also have two SERDES + * interfaces in the PHY address space. And these don't have a model + * number. But they are not PHYs, so we don't want to give them + * something a PHY driver will recognise. + */ + if (family == MV88E6XXX_FAMILY_6165) + return 0; + + model = family_model_table[family]; + if (model == MV88E_NA) + return 0; + + return mv88e6xxx_table[model].prod_num >> 4; +} + static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip) { const struct mv88e6xxx_info *info; diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index a57c8886f3ac..70c736788a68 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -47,6 +47,7 @@ enum mv88e6xxx_frame_mode { /* List of supported models */ enum mv88e6xxx_model { + MV88E_NA = 0, /* must be zero */ MV88E6085, MV88E6095, MV88E6097, @@ -90,6 +91,7 @@ enum mv88e6xxx_family { MV88E6XXX_FAMILY_6351, /* 6171 6175 6350 6351 */ MV88E6XXX_FAMILY_6352, /* 6172 6176 6240 6352 */ MV88E6XXX_FAMILY_6390, /* 6190 6190X 6191 6290 6390 6390X */ + MV88E6XXX_FAMILY_LAST }; struct mv88e6xxx_ops; diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index e26a5d663f8a..8018ddf7f316 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = { .get_stats = marvell_get_stats, }, { - .phy_id = MARVELL_PHY_ID_88E6390, + .phy_id = MARVELL_PHY_ID_88E6341_FAMILY, .phy_id_mask = MARVELL_PHY_ID_MASK, - .name = "Marvell 88E6390", + .name = "Marvell 88E6341 Family", + /* PHY_GBIT_FEATURES */ + .flags = PHY_POLL_CABLE_TEST, + .probe = m88e1510_probe, + .config_init = marvell_config_init, + .config_aneg = m88e6390_config_aneg, + .read_status = marvell_read_status, + .config_intr = marvell_config_intr, + .handle_interrupt = marvell_handle_interrupt, + .resume = genphy_resume, + .suspend = genphy_suspend, + .read_page = marvell_read_page, + .write_page = marvell_write_page, + .get_sset_count = marvell_get_sset_count, + .get_strings = marvell_get_strings, + .get_stats = marvell_get_stats, + .get_tunable = m88e1540_get_tunable, + .set_tunable = m88e1540_set_tunable, + .cable_test_start = marvell_vct7_cable_test_start, + .cable_test_tdr_start = marvell_vct5_cable_test_tdr_start, + .cable_test_get_status = marvell_vct7_cable_test_get_status, + }, + { + .phy_id = MARVELL_PHY_ID_88E6390_FAMILY, + .phy_id_mask = MARVELL_PHY_ID_MASK, + .name = "Marvell 88E6390 Family", /* PHY_GBIT_FEATURES */ .flags = PHY_POLL_CABLE_TEST, .probe = m88e6390_probe, @@ -3107,7 +3132,8 @@ 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_88E6390, 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_88E1340S, MARVELL_PHY_ID_MASK }, { MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK }, { } diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h index 52b1610eae68..c544b70dfbd2 100644 --- a/include/linux/marvell_phy.h +++ b/include/linux/marvell_phy.h @@ -28,11 +28,12 @@ /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */ #define MARVELL_PHY_ID_88E1111_FINISAR 0x01ff0cc0 -/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do +/* 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 */ -#define MARVELL_PHY_ID_88E6390 0x01410f90 +#define MARVELL_PHY_ID_88E6341_FAMILY 0x01410f41 +#define MARVELL_PHY_ID_88E6390_FAMILY 0x01410f90 #define MARVELL_PHY_FAMILY_ID(id) ((id) >> 4)
Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading"), Linux reports the temperature of Topaz hwmon as constant -75°C. This is because switches from the Topaz family (88E6141 / 88E6341) have the address of the temperature sensor register different from Peridot. This address is instead compatible with 88E1510 PHYs, as was used for Topaz before the above mentioned commit. Define a representative model for each switch family and add a mapping table for constructing PHY IDs for the internal PHYs (since they don't have a model number). Create a new PHY ID and a new PHY driver for Topaz' internal PHY. The only difference from Peridot's PHY driver is the HWMON probing method. Prior this change Topaz's internal PHY is detected by kernel as: PHY [...] driver [Marvell 88E6390] (irq=63) And afterwards as: PHY [...] driver [Marvell 88E6341 Family] (irq=63) Signed-off-by: Pali Rohár <pali@kernel.org> BugLink: https://github.com/globalscaletechnologies/linux/issues/1 Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading") Reviewed-by: Marek Behún <kabel@kernel.org> --- drivers/net/dsa/mv88e6xxx/chip.c | 56 ++++++++++++++++++++++---------- drivers/net/dsa/mv88e6xxx/chip.h | 2 ++ drivers/net/phy/marvell.c | 32 ++++++++++++++++-- include/linux/marvell_phy.h | 5 +-- 4 files changed, 72 insertions(+), 23 deletions(-)