On 12/18/18 10:53 PM, Heiner Kallweit wrote:
> PHY_HALTED and PHY_READY both are non-started states and quite similar.
> Major difference is that phy_start() changes from PHY_HALTED to
> PHY_RESUMING which doesn't reconfigure aneg (what PHY_UP does).
>
> There's no guarantee that PHY registers are completely untouched when
> waking up from power-down, e.g. after system suspend. Therefore it's
> safer to reconfigure aneg also when starting from PHY_HALTED. This can
> be achieved and state machine made simpler by making PHY_HALTED going
> to PHY_READY after having stopped everything. Then the only way up is
> over PHY_UP. Also let's warn in phy_start() if it's called from a
> state other than PHY_READY.
> As part of the change PHY_HALTED is renamed to PHY_HALT to reflect that
> it is a transition state.
Sorry for being uber nitpicky here, but a state machine is supposed to
contain.. state names, PHY_HALT is more of an action.
The PHY library is not particularly optimized at the moment to avoid
disrupting the link when there is not a need to, so if somehow the
register contents are lost because of a low power mode that the PHY has
entered, the PHY driver's resume function is responsible for bringing
things back online.
>
> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
> ---
> drivers/net/phy/phy.c | 41 +++++++++++++++++------------------------
> include/linux/phy.h | 16 ++++++++--------
> 2 files changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d33e7b3ca..2a69d947e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -47,12 +47,12 @@ static const char *phy_state_to_str(enum phy_state st)
> switch (st) {
> PHY_STATE_STR(DOWN)
> PHY_STATE_STR(READY)
> + PHY_STATE_STR(HALT)
> PHY_STATE_STR(UP)
> PHY_STATE_STR(RUNNING)
> PHY_STATE_STR(NOLINK)
> PHY_STATE_STR(FORCING)
> PHY_STATE_STR(CHANGELINK)
> - PHY_STATE_STR(HALTED)
> PHY_STATE_STR(RESUMING)
> }
>
> @@ -733,7 +733,7 @@ static void phy_error(struct phy_device *phydev)
> WARN_ON(1);
>
> mutex_lock(&phydev->lock);
> - phydev->state = PHY_HALTED;
> + phydev->state = PHY_HALT;
> mutex_unlock(&phydev->lock);
>
> phy_trigger_machine(phydev);
> @@ -859,16 +859,11 @@ void phy_stop(struct phy_device *phydev)
> if (phy_interrupt_is_valid(phydev))
> phy_disable_interrupts(phydev);
>
> - phydev->state = PHY_HALTED;
> + phydev->state = PHY_HALT;
>
> mutex_unlock(&phydev->lock);
>
> phy_state_machine(&phydev->state_queue.work);
> -
> - /* Cannot call flush_scheduled_work() here as desired because
> - * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
> - * will not reenable interrupts.
> - */
> }
> EXPORT_SYMBOL(phy_stop);
>
> @@ -888,29 +883,26 @@ void phy_start(struct phy_device *phydev)
>
> mutex_lock(&phydev->lock);
>
> - switch (phydev->state) {
> - case PHY_READY:
> - phydev->state = PHY_UP;
> - break;
> - case PHY_HALTED:
> + if (phydev->state == PHY_READY) {
> /* if phy was suspended, bring the physical link up again */
> __phy_resume(phydev);
>
> /* make sure interrupts are re-enabled for the PHY */
> if (phy_interrupt_is_valid(phydev)) {
> err = phy_enable_interrupts(phydev);
> - if (err < 0)
> - break;
> + if (err < 0) {
> + WARN_ON(1);
> + goto out;
> + }
> }
> -
> - phydev->state = PHY_RESUMING;
> - break;
> - default:
> - break;
> + phydev->state = PHY_UP;
> + phy_trigger_machine(phydev);
> + } else {
> + WARN(1, "called from state %s\n",
> + phy_state_to_str(phydev->state));
> }
> +out:
> mutex_unlock(&phydev->lock);
> -
> - phy_trigger_machine(phydev);
> }
> EXPORT_SYMBOL(phy_start);
>
> @@ -962,12 +954,13 @@ void phy_state_machine(struct work_struct *work)
> phy_link_down(phydev, false);
> }
> break;
> - case PHY_HALTED:
> + case PHY_HALT:
> if (phydev->link) {
> phydev->link = 0;
> phy_link_down(phydev, true);
> do_suspend = true;
> }
> + phydev->state = PHY_READY;
> break;
> }
>
> @@ -990,7 +983,7 @@ void phy_state_machine(struct work_struct *work)
> * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
> * between states from phy_mac_interrupt().
> *
> - * In state PHY_HALTED the PHY gets suspended, so rescheduling the
> + * In state PHY_HALT the PHY gets suspended, so rescheduling the
> * state machine would be pointless and possibly error prone when
> * called from phy_disconnect() synchronously.
> */
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index da039f211..21e553f51 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -288,38 +288,38 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus,
> int addr);
> *
> * NOLINK: PHY is up, but not currently plugged in.
> * - irq or timer will set RUNNING if link comes back
> - * - phy_stop moves to HALTED
> + * - phy_stop moves to HALT
> *
> * FORCING: PHY is being configured with forced settings
> * - if link is up, move to RUNNING
> * - If link is down, we drop to the next highest setting, and
> * retry (FORCING) after a timeout
> - * - phy_stop moves to HALTED
> + * - phy_stop moves to HALT
> *
> * RUNNING: PHY is currently up, running, and possibly sending
> * and/or receiving packets
> * - irq or timer will set NOLINK if link goes down
> - * - phy_stop moves to HALTED
> + * - phy_stop moves to HALT
> *
> * CHANGELINK: PHY experienced a change in link state
> * - timer moves to RUNNING if link
> * - timer moves to NOLINK if the link is down
> - * - phy_stop moves to HALTED
> + * - phy_stop moves to HALT
> *
> - * HALTED: PHY is up, but no polling or interrupts are done. Or
> + * HALT: PHY is up, but no polling or interrupts are done. Or
> * PHY is in an error state.
> *
> - * - phy_start moves to RESUMING
> + * - moves to READY
> *
> * RESUMING: PHY was halted, but now wants to run again.
> * - If we are forcing, or aneg is done, timer moves to RUNNING
> * - If aneg is not done, timer moves to AN
> - * - phy_stop moves to HALTED
> + * - phy_stop moves to HALT
> */
> enum phy_state {
> PHY_DOWN = 0,
> PHY_READY,
> - PHY_HALTED,
> + PHY_HALT,
> PHY_UP,
> PHY_RUNNING,
> PHY_NOLINK,
>
--
Florian
