| Message ID | 20231214201442.660447-3-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell10g: Firmware loading and LED support for 88X3310 | expand |
On Thu, Dec 14, 2023 at 09:14:40PM +0100, Tobias Waldekranz wrote: > On devices which are hardware strapped to start powered down (PDSTATE > == 1), make sure that we clear the power-down bit on all units > affected by this setting. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > drivers/net/phy/marvell10g.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > index 83233b30d7b0..1c1333d867fb 100644 > --- a/drivers/net/phy/marvell10g.c > +++ b/drivers/net/phy/marvell10g.c > @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev) > > static int mv3310_power_up(struct phy_device *phydev) > { > + static const u16 resets[][2] = { > + { MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1 }, > + { MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1 }, This is not necessary. The documentation states that the power down bit found at each of these is the same physical bit appearing in two different locations. So only one is necessary. > + { MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1 }, > + { MDIO_MMD_PMAPMD, MDIO_CTRL1 }, > + { MDIO_MMD_VEND2, MV_V2_PORT_CTRL }, > + }; > struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); > - int ret; > + int i, ret; > > - ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, > - MV_V2_PORT_CTRL_PWRDOWN); > + for (i = 0; i < ARRAY_SIZE(resets); i++) { > + ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1], > + MV_V2_PORT_CTRL_PWRDOWN); While MV_V2_PORT_CTRL_PWRDOWN may correspond with the correct bit for the MDIO CTRL1 register, we have MDIO_CTRL1_LPOWER which describes this bit. Probably the simplest solution would be to leave the existing phy_clear_bits_mmd(), remove the vendor 2 entry from the table, and run through that table first. Lastly, how does this impact a device which has firmware, and the firmware manages the power-down state (the manual states that unused blocks will be powered down - I assume by the firmware.) If this causes blocks which had been powered down by the firmware because they're not being used to then be powered up, that is a regression. Please check that this is not the case.
On fre, dec 15, 2023 at 15:44, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Thu, Dec 14, 2023 at 09:14:40PM +0100, Tobias Waldekranz wrote: >> On devices which are hardware strapped to start powered down (PDSTATE >> == 1), make sure that we clear the power-down bit on all units >> affected by this setting. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> drivers/net/phy/marvell10g.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c >> index 83233b30d7b0..1c1333d867fb 100644 >> --- a/drivers/net/phy/marvell10g.c >> +++ b/drivers/net/phy/marvell10g.c >> @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev) >> >> static int mv3310_power_up(struct phy_device *phydev) >> { >> + static const u16 resets[][2] = { >> + { MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1 }, >> + { MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1 }, > > This is not necessary. The documentation states that the power down > bit found at each of these is the same physical bit appearing in two > different locations. So only one is necessary. Right, I'll remove the entry for 1000BASE-X in v2. >> + { MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1 }, >> + { MDIO_MMD_PMAPMD, MDIO_CTRL1 }, >> + { MDIO_MMD_VEND2, MV_V2_PORT_CTRL }, >> + }; >> struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); >> - int ret; >> + int i, ret; >> >> - ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, >> - MV_V2_PORT_CTRL_PWRDOWN); >> + for (i = 0; i < ARRAY_SIZE(resets); i++) { >> + ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1], >> + MV_V2_PORT_CTRL_PWRDOWN); > > While MV_V2_PORT_CTRL_PWRDOWN may correspond with the correct bit for > the MDIO CTRL1 register, we have MDIO_CTRL1_LPOWER which describes > this bit. Probably the simplest solution would be to leave the > existing phy_clear_bits_mmd(), remove the vendor 2 entry from the > table, and run through that table first. Yes, I'll fix this in v2. > Lastly, how does this impact a device which has firmware, and the > firmware manages the power-down state (the manual states that unused > blocks will be powered down - I assume by the firmware.) If this > causes blocks which had been powered down by the firmware because > they're not being used to then be powered up, that is a regression. > Please check that this is not the case. This will be very hard for me to test, as I only have access to boards without dedicated FLASHes. That said, I don't think I understand how this is related to how the devices load their firmware. As I understand it, we should pick up the device in the exact same state after the MDIO load as we would if it had booted on its own, via a serial FLASH. The selection of PDSTATE, based on the sample-at-reset pins, is independent of how firmware is loaded. From the manual: The following registers can be set to force the units to power down. I interpret this as the power-down bits only acting as gates to stop firmware from powering up a particular unit. Conversely, clearing one of these bits merely indicates that the firmware is free to power up the unit in question. On a device strapped to PDSTATE==0, I would expect all of these bits to already be cleared, since the manual states that the value of PDSTATE is latched into all these bits at reset.
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 83233b30d7b0..1c1333d867fb 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev) static int mv3310_power_up(struct phy_device *phydev) { + static const u16 resets[][2] = { + { MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1 }, + { MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1 }, + { MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1 }, + { MDIO_MMD_PMAPMD, MDIO_CTRL1 }, + { MDIO_MMD_VEND2, MV_V2_PORT_CTRL }, + }; struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); - int ret; + int i, ret; - ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, - MV_V2_PORT_CTRL_PWRDOWN); + for (i = 0; i < ARRAY_SIZE(resets); i++) { + ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1], + MV_V2_PORT_CTRL_PWRDOWN); + if (ret) + break; + } /* Sometimes, the power down bit doesn't clear immediately, and * a read of this register causes the bit not to clear. Delay
On devices which are hardware strapped to start powered down (PDSTATE == 1), make sure that we clear the power-down bit on all units affected by this setting. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/phy/marvell10g.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)