| Message ID | 20231214201442.660447-3-tobias@waldekranz.com |
|---|---|
| State | New |
| 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.
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(-)