On fre, dec 15, 2023 at 15:44, "Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> ---
>> 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.
