Re: [PATCH 6/7] net: phy: at803x: add disable hibernation mode support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 10.07.23 12:53, Marco Felsch wrote:
> On 23-07-10, Ahmad Fatoum wrote:
>> On 10.07.23 08:36, Marco Felsch wrote:
>>> This commit ports Linux commit:
>>>
>>> | commit 9ecf04016c87bcb33b44e24489d33618e2592f41
>>> | Author: Wei Fang <wei.fang@nxp.com>
>>> | Date:   Thu Aug 18 11:00:54 2022 +0800
>>> |
>>> |     net: phy: at803x: add disable hibernation mode support
>>> |
>>> |     When the cable is unplugged, the Atheros AR803x PHYs will enter
>>> |     hibernation mode after about 10 seconds if the hibernation mode
>>> |     is enabled and will not provide any clock to the MAC. But for
>>> |     some MACs, this feature might cause unexpected issues due to the
>>> |     logic of MACs.
>>> |     Taking SYNP MAC (stmmac) as an example, if the cable is unplugged
>>> |     and the "eth0" interface is down, the AR803x PHY will enter
>>> |     hibernation mode. Then perform the "ifconfig eth0 up" operation,
>>> |     the stmmac can't be able to complete the software reset operation
>>> |     and fail to init it's own DMA. Therefore, the "eth0" interface is
>>> |     failed to ifconfig up. Why does it cause this issue? The truth is
>>> |     that the software reset operation of the stmmac is designed to
>>> |     depend on the RX_CLK of PHY.
>>> |     So, this patch offers an option for the user to determine whether
>>> |     to disable the hibernation mode of AR803x PHYs.
>>> |
>>> |     Signed-off-by: Wei Fang <wei.fang@nxp.com>
>>> |     Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>> |     Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>>
>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>
>> IMO we should just makes AT803X_DISABLE_HIBERNATION_MODE the default and
>> never hibernate the PHY. Let Linux worry about that.
> 
> Got your point and I'm not sure. At least for drivers I like the sources
> to bin in sync with Linux which allows porting fixes/features more
> easily.

You can keep code mostly as-is and just replace the DT property check
with a comment why we do this unconditionally.

> Also since the API (dt-bindings) are there, so why not just
> copy'n'paste from Linux?

We don't care much for power saving during barebox runtime. We turn on
whatever clocks are necessary and leave it to Linux to power down
unneeded peripherals. Same thing should apply to the PHY. If the PHY
has a power saving mode that breaks some MACs, why not just disable
the power saving mode? It's better for code size anyway.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>>> ---
>>>  drivers/net/phy/at803x.c | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>>> index cc425c318f..d8a9c3588f 100644
>>> --- a/drivers/net/phy/at803x.c
>>> +++ b/drivers/net/phy/at803x.c
>>> @@ -32,6 +32,9 @@
>>>  #define AT803X_DEBUG_REG_5			0x05
>>>  #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
>>>  
>>> +#define AT803X_DEBUG_REG_HIB_CTRL		0x0b
>>> +#define   AT803X_DEBUG_HIB_CTRL_PS_HIB_EN	BIT(15)
>>> +
>>>  /* AT803x supports either the XTAL input pad, an internal PLL or the
>>>   * DSP as clock reference for the clock output pad. The XTAL reference
>>>   * is only used for 25 MHz output, all other frequencies need the PLL.
>>> @@ -140,6 +143,20 @@ static int at803x_disable_tx_delay(struct phy_device *phydev)
>>>  				     AT803X_DEBUG_TX_CLK_DLY_EN, 0);
>>>  }
>>>  
>>> +static int at803x_hibernation_mode_config(struct phy_device *phydev)
>>> +{
>>> +	struct at803x_priv *priv = phydev->priv;
>>> +
>>> +	/* The default after hardware reset is hibernation mode enabled. After
>>> +	 * software reset, the value is retained.
>>> +	 */
>>> +	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
>>> +		return 0;
>>> +
>>> +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
>>> +				     AT803X_DEBUG_HIB_CTRL_PS_HIB_EN, 0);
>>> +}
>>> +
>>>  static bool at803x_match_phy_id(struct phy_device *phydev, u32 phy_id)
>>>  {
>>>  	struct phy_driver *drv = to_phy_driver(phydev->dev.driver);
>>> @@ -160,6 +177,9 @@ static int at803x_parse_dt(struct phy_device *phydev)
>>>  	if (of_property_read_bool(node, "qca,disable-smarteee"))
>>>  		priv->flags |= AT803X_DISABLE_SMARTEEE;
>>>  
>>> +	if (of_property_read_bool(node, "qca,disable-hibernation-mode"))
>>> +		priv->flags |= AT803X_DISABLE_HIBERNATION_MODE;
>>> +
>>>  	if (!of_property_read_u32(node, "qca,smarteee-tw-us-1g", &tw)) {
>>>  		if (!tw || tw > 255) {
>>>  			phydev_err(phydev, "invalid qca,smarteee-tw-us-1g\n");
>>> @@ -341,6 +361,10 @@ static int at803x_config_init(struct phy_device *phydev)
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>> +	ret = at803x_hibernation_mode_config(phydev);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>>  	return 0;
>>>  }
>>>  
>>
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |