[PATCH] [RFC] net: phy: smsc: Add magnetics VIO regulator support

[PATCH] [RFC] net: phy: smsc: Add magnetics VIO regulator support

[Marek Vasut]

Add support for controlling regulator powering the magnetics. In case
the interface is down, it is possible to save considerable power by
turning the regulator supplying the magnetics off.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 33372756a451..edc2bd7d8100 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -20,6 +20,9 @@
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/netdevice.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/consumer.h>
 #include <linux/smscphy.h>
 
 /* Vendor-specific PHY Definitions */
@@ -46,6 +49,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
 struct smsc_phy_priv {
 	bool energy_enable;
 	struct clk *refclk;
+	struct regulator *vddio;
 };
 
 static int smsc_phy_ack_interrupt(struct phy_device *phydev)
@@ -288,6 +292,20 @@ static void smsc_get_stats(struct phy_device *phydev,
 		data[i] = smsc_get_stat(phydev, i);
 }
 
+static void smsc_link_change_notify(struct phy_device *phydev)
+{
+	struct smsc_phy_priv *priv = phydev->priv;
+
+	if (!priv->vddio)
+		return;
+
+	if (phydev->state == PHY_HALTED)
+		regulator_disable(priv->vddio);
+
+	if (phydev->state == PHY_NOLINK)
+		regulator_enable(priv->vddio);
+}
+
 static void smsc_phy_remove(struct phy_device *phydev)
 {
 	struct smsc_phy_priv *priv = phydev->priv;
@@ -309,6 +327,10 @@ static int smsc_phy_probe(struct phy_device *phydev)
 
 	priv->energy_enable = true;
 
+	priv->vddio = devm_regulator_get_optional(&phydev->mdio.dev, "vddio");
+	if (IS_ERR(priv->vddio))
+		return PTR_ERR(priv->vddio);
+
 	if (of_property_read_bool(of_node, "smsc,disable-energy-detect"))
 		priv->energy_enable = false;
 
@@ -432,7 +454,7 @@ static struct phy_driver smsc_phy_driver[] = {
 	.name		= "SMSC LAN8710/LAN8720",
 
 	/* PHY_BASIC_FEATURES */
-
+	.link_change_notify = smsc_link_change_notify,
 	.probe		= smsc_phy_probe,
 	.remove		= smsc_phy_remove,
 
-- 
2.29.2

Re: [PATCH] [RFC] net: phy: smsc: Add magnetics VIO regulator support

[Andrew Lunn]

> +static void smsc_link_change_notify(struct phy_device *phydev)
> +{
> +	struct smsc_phy_priv *priv = phydev->priv;
> +
> +	if (!priv->vddio)
> +		return;
> +
> +	if (phydev->state == PHY_HALTED)
> +		regulator_disable(priv->vddio);
> +
> +	if (phydev->state == PHY_NOLINK)
> +		regulator_enable(priv->vddio);

NOLINK is an interesting choice. Could you explain that please.

I fear this is not going to be very robust to state machine
changes. And since it is hidden away in a driver, it is going to be
forgotten about. You might want to think about making it more robust.

	  Andrew

Re: [PATCH] [RFC] net: phy: smsc: Add magnetics VIO regulator support

[Marek Vasut]

On 1/5/21 6:38 PM, Andrew Lunn wrote:
>> +static void smsc_link_change_notify(struct phy_device *phydev)
>> +{
>> +	struct smsc_phy_priv *priv = phydev->priv;
>> +
>> +	if (!priv->vddio)
>> +		return;
>> +
>> +	if (phydev->state == PHY_HALTED)
>> +		regulator_disable(priv->vddio);
>> +
>> +	if (phydev->state == PHY_NOLINK)
>> +		regulator_enable(priv->vddio);
> 
> NOLINK is an interesting choice. Could you explain that please.

It's the first state after interface is up.

> I fear this is not going to be very robust to state machine
> changes. And since it is hidden away in a driver, it is going to be
> forgotten about. You might want to think about making it more robust.

I marked the patch as RFC because I would like input on how to implement 
this properly. Note that since the regulator supplies the magnetics, 
which might be shared between multiple ports with different PHYs, I 
don't think this code should even be in the PHY driver, but somewhere 
else -- but I don't know where.

Re: [PATCH] [RFC] net: phy: smsc: Add magnetics VIO regulator support

[Andrew Lunn]

On Tue, Jan 05, 2021 at 06:53:48PM +0100, Marek Vasut wrote:
> On 1/5/21 6:38 PM, Andrew Lunn wrote:
> > > +static void smsc_link_change_notify(struct phy_device *phydev)
> > > +{
> > > +	struct smsc_phy_priv *priv = phydev->priv;
> > > +
> > > +	if (!priv->vddio)
> > > +		return;
> > > +
> > > +	if (phydev->state == PHY_HALTED)
> > > +		regulator_disable(priv->vddio);
> > > +
> > > +	if (phydev->state == PHY_NOLINK)
> > > +		regulator_enable(priv->vddio);
> > 
> > NOLINK is an interesting choice. Could you explain that please.
> 
> It's the first state after interface is up.

No, not really. phy_start() actually sets it to PHY_UP. When the state
machine runs, it kicks off auto-neg and immediately reads the link
state. If the link is down, it transitions to PHY_NOLINK, at which
point this code will enable the regulator.

> > I fear this is not going to be very robust to state machine
> > changes. And since it is hidden away in a driver, it is going to be
> > forgotten about. You might want to think about making it more robust.
> 
> I marked the patch as RFC because I would like input on how to implement
> this properly. Note that since the regulator supplies the magnetics, which
> might be shared between multiple ports with different PHYs, I don't think
> this code should even be in the PHY driver, but somewhere else -- but I
> don't know where.

Being shared should not be a problem. The regulator API does reference
counting. Any one driver turning the regulator on will enable it. But
it will not be turned off until all the drivers disable it after
enabling it. But that also means you need to balance the calls to
regulator_enable() and regulator_disable().

If for whatever reason this function is called for PHY_HALTED more
times than for PHY_NOLINK, the counter can go negative, and bad things
would happen. So i would actually had a bool to smsc_phy_priv
indicating if the regulator has been enabled. And for each
phydev->state, decide if the regulator should be enabled, check if it
is enabled according to the bool, and enable it is not. Same with
states which indicate it should be disabled. The code is then not
dependent on specific transitions, but on actual states. That should
be more robust to changes.

You also need to think about this regulator being shared. Say some
other PHY has enabled the regulator. phy_start() might be able to skip
PHY_NOLINK state and so this PHY never calls regulator_enable(). If
that other PHY is then configured down, it will disable the regulator,
and this PHY looses link. That probably is enough for this PHY to
re-enable the regulator, but it is not ideal.

	  Andrew

Re: [PATCH] [RFC] net: phy: smsc: Add magnetics VIO regulator support

[Marek Vasut]

On 1/5/21 8:03 PM, Andrew Lunn wrote:
> On Tue, Jan 05, 2021 at 06:53:48PM +0100, Marek Vasut wrote:
>> On 1/5/21 6:38 PM, Andrew Lunn wrote:
>>>> +static void smsc_link_change_notify(struct phy_device *phydev)
>>>> +{
>>>> +	struct smsc_phy_priv *priv = phydev->priv;
>>>> +
>>>> +	if (!priv->vddio)
>>>> +		return;
>>>> +
>>>> +	if (phydev->state == PHY_HALTED)
>>>> +		regulator_disable(priv->vddio);
>>>> +
>>>> +	if (phydev->state == PHY_NOLINK)
>>>> +		regulator_enable(priv->vddio);
>>>
>>> NOLINK is an interesting choice. Could you explain that please.
>>
>> It's the first state after interface is up.
> 
> No, not really. phy_start() actually sets it to PHY_UP. When the state
> machine runs, it kicks off auto-neg and immediately reads the link
> state. If the link is down, it transitions to PHY_NOLINK, at which
> point this code will enable the regulator.
> 
>>> I fear this is not going to be very robust to state machine
>>> changes. And since it is hidden away in a driver, it is going to be
>>> forgotten about. You might want to think about making it more robust.
>>
>> I marked the patch as RFC because I would like input on how to implement
>> this properly. Note that since the regulator supplies the magnetics, which
>> might be shared between multiple ports with different PHYs, I don't think
>> this code should even be in the PHY driver, but somewhere else -- but I
>> don't know where.
> 
> Being shared should not be a problem. The regulator API does reference
> counting. Any one driver turning the regulator on will enable it. But
> it will not be turned off until all the drivers disable it after
> enabling it. But that also means you need to balance the calls to
> regulator_enable() and regulator_disable().
> 
> If for whatever reason this function is called for PHY_HALTED more
> times than for PHY_NOLINK, the counter can go negative, and bad things
> would happen. So i would actually had a bool to smsc_phy_priv
> indicating if the regulator has been enabled. And for each
> phydev->state, decide if the regulator should be enabled, check if it
> is enabled according to the bool, and enable it is not. Same with
> states which indicate it should be disabled. The code is then not
> dependent on specific transitions, but on actual states. That should
> be more robust to changes.
> 
> You also need to think about this regulator being shared. Say some
> other PHY has enabled the regulator. phy_start() might be able to skip
> PHY_NOLINK state and so this PHY never calls regulator_enable(). If
> that other PHY is then configured down, it will disable the regulator,
> and this PHY looses link. That probably is enough for this PHY to
> re-enable the regulator, but it is not ideal.

I think you are completely missing the point, the regulator is just an 
implementation detail. I am more interested in the implementation 
itself, which I suspect should not even be in the PHY driver, but rather 
somewhere closer to the core (where?), because the supply to magnetics 
is not part of the PHY, any PHY can be used with magnetics which need a 
regulator.

Re: [PATCH] [RFC] net: phy: smsc: Add magnetics VIO regulator support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]

Hi Marek,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master linus/master v5.11-rc2 next-20210104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marek-Vasut/net-phy-smsc-Add-magnetics-VIO-regulator-support/20210106-001946
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 4bfc4714849d005e6835bcffa3c29ebd6e5ee35d
config: x86_64-randconfig-a006-20210105 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5c951623bc8965fa1e89660f2f5f4a2944e4981a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/014a5aebb8dfa7edde7e312123a45d99372988b7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/net-phy-smsc-Add-magnetics-VIO-regulator-support/20210106-001946
        git checkout 014a5aebb8dfa7edde7e312123a45d99372988b7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/phy/smsc.c:306:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
                   regulator_enable(priv->vddio);
                   ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~
   1 warning generated.


vim +/warn_unused_result +306 drivers/net/phy/smsc.c

   294	
   295	static void smsc_link_change_notify(struct phy_device *phydev)
   296	{
   297		struct smsc_phy_priv *priv = phydev->priv;
   298	
   299		if (!priv->vddio)
   300			return;
   301	
   302		if (phydev->state == PHY_HALTED)
   303			regulator_disable(priv->vddio);
   304	
   305		if (phydev->state == PHY_NOLINK)
 > 306			regulator_enable(priv->vddio);
   307	}
   308	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 44967 bytes --]