diff mbox series

[net-next,v4,1/2] net: phy: marvell10g: implement suspend/resume callbacks

Message ID 20190402131029.26880-2-antoine.tenart@bootlin.com
State Accepted
Delegated to: David Miller
Headers show
Series net: phy: marvell10g: implement suspend/resume callbacks | expand

Commit Message

Antoine Tenart April 2, 2019, 1:10 p.m. UTC
This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
are powered down) when the PHY isn't used.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Heiner Kallweit April 2, 2019, 6:17 p.m. UTC | #1
On 02.04.2019 15:10, Antoine Tenart wrote:
> This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
> three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
> are powered down) when the PHY isn't used.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  drivers/net/phy/marvell10g.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 80678919641d..9ee033c8a12b 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -51,6 +51,8 @@ enum {
>  	MV_AN_STAT1000		= 0x8001, /* 1000base-T status register */
>  
>  	/* Vendor2 MMD registers */
> +	MV_V2_PORT_CTRL		= 0xf001,
> +	MV_V2_PORT_CTRL_PWRDOWN = 0x0800,

If this driver is touched again I think it would be good to change all
such constants to BIT() and GENMASK(), ideally combined with the macros
from bitfields.h. This makes it much easier to check the code against the
datasheet. Apart from that:

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>
Russell King - ARM Linux admin April 2, 2019, 10:10 p.m. UTC | #2
On Tue, Apr 02, 2019 at 08:17:16PM +0200, Heiner Kallweit wrote:
> On 02.04.2019 15:10, Antoine Tenart wrote:
> > This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
> > three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
> > are powered down) when the PHY isn't used.
> > 
> > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > ---
> >  drivers/net/phy/marvell10g.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index 80678919641d..9ee033c8a12b 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -51,6 +51,8 @@ enum {
> >  	MV_AN_STAT1000		= 0x8001, /* 1000base-T status register */
> >  
> >  	/* Vendor2 MMD registers */
> > +	MV_V2_PORT_CTRL		= 0xf001,
> > +	MV_V2_PORT_CTRL_PWRDOWN = 0x0800,
> 
> If this driver is touched again I think it would be good to change all
> such constants to BIT() and GENMASK(), ideally combined with the macros
> from bitfields.h. This makes it much easier to check the code against the
> datasheet. Apart from that:

Specifically, which constants are you talking about?

I think there's only MV_PCS_PAIRSWAP_MASK and MV_V2_TEMP_CTRL_MASK,
which would be confusing to change given that the following definitions
are values for the masked field.

However, MV_V2_PORT_CTRL_PWRDOWN should be defined using BIT() in any
case.
Heiner Kallweit April 3, 2019, 5:09 a.m. UTC | #3
On 03.04.2019 00:10, Russell King - ARM Linux admin wrote:
> On Tue, Apr 02, 2019 at 08:17:16PM +0200, Heiner Kallweit wrote:
>> On 02.04.2019 15:10, Antoine Tenart wrote:
>>> This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
>>> three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
>>> are powered down) when the PHY isn't used.
>>>
>>> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
>>> ---
>>>  drivers/net/phy/marvell10g.c | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>>> index 80678919641d..9ee033c8a12b 100644
>>> --- a/drivers/net/phy/marvell10g.c
>>> +++ b/drivers/net/phy/marvell10g.c
>>> @@ -51,6 +51,8 @@ enum {
>>>  	MV_AN_STAT1000		= 0x8001, /* 1000base-T status register */
>>>  
>>>  	/* Vendor2 MMD registers */
>>> +	MV_V2_PORT_CTRL		= 0xf001,
>>> +	MV_V2_PORT_CTRL_PWRDOWN = 0x0800,
>>
>> If this driver is touched again I think it would be good to change all
>> such constants to BIT() and GENMASK(), ideally combined with the macros
>> from bitfields.h. This makes it much easier to check the code against the
>> datasheet. Apart from that:
> 
> Specifically, which constants are you talking about?
> 
> I think there's only MV_PCS_PAIRSWAP_MASK and MV_V2_TEMP_CTRL_MASK,
> which would be confusing to change given that the following definitions
> are values for the masked field.
> 
Exactly, MV_V2_TEMP_CTRL_MASK is a good example. My personal preference is
to define the mask as GENMASK(15, 14) and the field values as 0 and 3.
Then it's aligned with the datasheet that says:
15:14 Temperature Sense Enable, 11 = Disable
Macros FIELD_GET and FIELD_PREP are perfect to deal with such fields.

> However, MV_V2_PORT_CTRL_PWRDOWN should be defined using BIT() in any
> case.
>
Antoine Tenart April 3, 2019, 8:46 a.m. UTC | #4
Hi,

On Wed, Apr 03, 2019 at 07:09:55AM +0200, Heiner Kallweit wrote:
> On 03.04.2019 00:10, Russell King - ARM Linux admin wrote:
> > On Tue, Apr 02, 2019 at 08:17:16PM +0200, Heiner Kallweit wrote:
> >> On 02.04.2019 15:10, Antoine Tenart wrote:
> >>> This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
> >>> three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
> >>> are powered down) when the PHY isn't used.
> >>>
> >>> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> >>> ---
> >>>  drivers/net/phy/marvell10g.c | 12 +++++++++++-
> >>>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> >>> index 80678919641d..9ee033c8a12b 100644
> >>> --- a/drivers/net/phy/marvell10g.c
> >>> +++ b/drivers/net/phy/marvell10g.c
> >>> @@ -51,6 +51,8 @@ enum {
> >>>  	MV_AN_STAT1000		= 0x8001, /* 1000base-T status register */
> >>>  
> >>>  	/* Vendor2 MMD registers */
> >>> +	MV_V2_PORT_CTRL		= 0xf001,
> >>> +	MV_V2_PORT_CTRL_PWRDOWN = 0x0800,
> >>
> >> If this driver is touched again I think it would be good to change all
> >> such constants to BIT() and GENMASK(), ideally combined with the macros
> >> from bitfields.h. This makes it much easier to check the code against the
> >> datasheet. Apart from that:
> > 
> > Specifically, which constants are you talking about?
> > 
> > I think there's only MV_PCS_PAIRSWAP_MASK and MV_V2_TEMP_CTRL_MASK,
> > which would be confusing to change given that the following definitions
> > are values for the masked field.
> > 
> Exactly, MV_V2_TEMP_CTRL_MASK is a good example. My personal preference is
> to define the mask as GENMASK(15, 14) and the field values as 0 and 3.
> Then it's aligned with the datasheet that says:
> 15:14 Temperature Sense Enable, 11 = Disable
> Macros FIELD_GET and FIELD_PREP are perfect to deal with such fields.

I agree, I didn't used that to be consistent with what was already done
in the driver.

> > However, MV_V2_PORT_CTRL_PWRDOWN should be defined using BIT() in any
> > case.

Shouldn't MV_PCS_PAIRSWAP_AB also be defined using BIT()?

More generally, we could have all the register definitions using the 0x
values, the masks using GENMASK() and the values using a combination of
BIT() and (0x... << y). That would match what's usually done in other
drivers and improve the readability. (But I also recall being told not
to use GENMASK in net/, so it's up to you to decide).

I can send a following up patch if needed and if we agree on this.

Antoine
Russell King - ARM Linux admin April 3, 2019, 9:40 a.m. UTC | #5
On Wed, Apr 03, 2019 at 10:46:14AM +0200, Antoine Tenart wrote:
> Hi,
> 
> On Wed, Apr 03, 2019 at 07:09:55AM +0200, Heiner Kallweit wrote:
> > On 03.04.2019 00:10, Russell King - ARM Linux admin wrote:
> > > On Tue, Apr 02, 2019 at 08:17:16PM +0200, Heiner Kallweit wrote:
> > >> On 02.04.2019 15:10, Antoine Tenart wrote:
> > >>> This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
> > >>> three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
> > >>> are powered down) when the PHY isn't used.
> > >>>
> > >>> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > >>> ---
> > >>>  drivers/net/phy/marvell10g.c | 12 +++++++++++-
> > >>>  1 file changed, 11 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > >>> index 80678919641d..9ee033c8a12b 100644
> > >>> --- a/drivers/net/phy/marvell10g.c
> > >>> +++ b/drivers/net/phy/marvell10g.c
> > >>> @@ -51,6 +51,8 @@ enum {
> > >>>  	MV_AN_STAT1000		= 0x8001, /* 1000base-T status register */
> > >>>  
> > >>>  	/* Vendor2 MMD registers */
> > >>> +	MV_V2_PORT_CTRL		= 0xf001,
> > >>> +	MV_V2_PORT_CTRL_PWRDOWN = 0x0800,
> > >>
> > >> If this driver is touched again I think it would be good to change all
> > >> such constants to BIT() and GENMASK(), ideally combined with the macros
> > >> from bitfields.h. This makes it much easier to check the code against the
> > >> datasheet. Apart from that:
> > > 
> > > Specifically, which constants are you talking about?
> > > 
> > > I think there's only MV_PCS_PAIRSWAP_MASK and MV_V2_TEMP_CTRL_MASK,
> > > which would be confusing to change given that the following definitions
> > > are values for the masked field.
> > > 
> > Exactly, MV_V2_TEMP_CTRL_MASK is a good example. My personal preference is
> > to define the mask as GENMASK(15, 14) and the field values as 0 and 3.
> > Then it's aligned with the datasheet that says:
> > 15:14 Temperature Sense Enable, 11 = Disable

Sorry, I don't see that.  You still have to convert between what the
data sheet says (binary) and the values used in C code (hex or decimal).
To me, it would be natural to state the above as 0xc000 rather than
stating it as '3' in the code with a GENMASK defining a mask for the
appropriate fields, and then have to check all over the place that
the right FIELD_* macros are used.  This seems _way_ more complex than
it needs to be.

> > Macros FIELD_GET and FIELD_PREP are perfect to deal with such fields.
> 
> I agree, I didn't used that to be consistent with what was already done
> in the driver.
> 
> > > However, MV_V2_PORT_CTRL_PWRDOWN should be defined using BIT() in any
> > > case.
> 
> Shouldn't MV_PCS_PAIRSWAP_AB also be defined using BIT()?

It is this that makes me utterly detest BIT().  It seems folk just
look at the value and think "it defines a single bit, it must use
BIT()" without thinking that it might be a field.  So we end up
with people inappropriately proposing to change stuff to BIT().

So no.

> More generally, we could have all the register definitions using the 0x
> values, the masks using GENMASK() and the values using a combination of
> BIT() and (0x... << y). That would match what's usually done in other
> drivers and improve the readability. (But I also recall being told not
> to use GENMASK in net/, so it's up to you to decide).

That difference is why I don't like GENMASK().  We end up with masks
defined using GENMASK() and their bitfields defined a completely
different way - there is no consistency, and it is not obvious that
the masks and the field values are related.

In my opinion, BIT() is only marginally useful, the rest just makes
things _less_ obviously correct.
diff mbox series

Patch

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 80678919641d..9ee033c8a12b 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -51,6 +51,8 @@  enum {
 	MV_AN_STAT1000		= 0x8001, /* 1000base-T status register */
 
 	/* Vendor2 MMD registers */
+	MV_V2_PORT_CTRL		= 0xf001,
+	MV_V2_PORT_CTRL_PWRDOWN = 0x0800,
 	MV_V2_TEMP_CTRL		= 0xf08a,
 	MV_V2_TEMP_CTRL_MASK	= 0xc000,
 	MV_V2_TEMP_CTRL_SAMPLE	= 0x0000,
@@ -229,11 +231,19 @@  static int mv3310_probe(struct phy_device *phydev)
 
 static int mv3310_suspend(struct phy_device *phydev)
 {
-	return 0;
+	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				MV_V2_PORT_CTRL_PWRDOWN);
 }
 
 static int mv3310_resume(struct phy_device *phydev)
 {
+	int ret;
+
+	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				 MV_V2_PORT_CTRL_PWRDOWN);
+	if (ret)
+		return ret;
+
 	return mv3310_hwmon_config(phydev, true);
 }