* [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks @ 2019-04-02 13:10 Antoine Tenart 2019-04-02 13:10 ` [PATCH net-next v4 1/2] " Antoine Tenart ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Antoine Tenart @ 2019-04-02 13:10 UTC (permalink / raw) To: davem, linux, andrew, f.fainelli, hkallweit1 Cc: Antoine Tenart, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw Hello, This series implements the suspend/resume callbacks in the marvell10g PHY driver. Thanks, Antoine Since v3: - Use the new phy_set/clear_bits_mmd() instead of phy_modify_mmd(). - Use VEND2.f001.11 to power down the port instead of the per-mode LPOWER. Since v2: - Removed the third patch, setting the PHY in low power by default, as the change was controversial. - Rebased on the latest net-next. Since v1: - Fixed a mix up in the patches where two implementations of the suspend/resume callbacks were kept in the driver. - Rebased on the latest net-next. Antoine Tenart (2): net: phy: marvell10g: implement suspend/resume callbacks net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210 drivers/net/phy/marvell10g.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v4 1/2] net: phy: marvell10g: implement suspend/resume callbacks 2019-04-02 13:10 [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks Antoine Tenart @ 2019-04-02 13:10 ` Antoine Tenart 2019-04-02 18:17 ` Heiner Kallweit 2019-04-02 13:10 ` [PATCH net-next v4 2/2] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210 Antoine Tenart 2019-04-04 4:46 ` [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks David Miller 2 siblings, 1 reply; 9+ messages in thread From: Antoine Tenart @ 2019-04-02 13:10 UTC (permalink / raw) To: davem, linux, andrew, f.fainelli, hkallweit1 Cc: Antoine Tenart, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw 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, 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); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v4 1/2] net: phy: marvell10g: implement suspend/resume callbacks 2019-04-02 13:10 ` [PATCH net-next v4 1/2] " Antoine Tenart @ 2019-04-02 18:17 ` Heiner Kallweit 2019-04-02 22:10 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 9+ messages in thread From: Heiner Kallweit @ 2019-04-02 18:17 UTC (permalink / raw) To: Antoine Tenart, davem, linux, andrew, f.fainelli Cc: netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v4 1/2] net: phy: marvell10g: implement suspend/resume callbacks 2019-04-02 18:17 ` Heiner Kallweit @ 2019-04-02 22:10 ` Russell King - ARM Linux admin 2019-04-03 5:09 ` Heiner Kallweit 0 siblings, 1 reply; 9+ messages in thread From: Russell King - ARM Linux admin @ 2019-04-02 22:10 UTC (permalink / raw) To: Heiner Kallweit Cc: Antoine Tenart, davem, andrew, f.fainelli, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v4 1/2] net: phy: marvell10g: implement suspend/resume callbacks 2019-04-02 22:10 ` Russell King - ARM Linux admin @ 2019-04-03 5:09 ` Heiner Kallweit 2019-04-03 8:46 ` Antoine Tenart 0 siblings, 1 reply; 9+ messages in thread From: Heiner Kallweit @ 2019-04-03 5:09 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Antoine Tenart, davem, andrew, f.fainelli, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw 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. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v4 1/2] net: phy: marvell10g: implement suspend/resume callbacks 2019-04-03 5:09 ` Heiner Kallweit @ 2019-04-03 8:46 ` Antoine Tenart 2019-04-03 9:40 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 9+ messages in thread From: Antoine Tenart @ 2019-04-03 8:46 UTC (permalink / raw) To: Heiner Kallweit Cc: Russell King - ARM Linux admin, Antoine Tenart, davem, andrew, f.fainelli, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw 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 -- Antoine Ténart, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v4 1/2] net: phy: marvell10g: implement suspend/resume callbacks 2019-04-03 8:46 ` Antoine Tenart @ 2019-04-03 9:40 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 9+ messages in thread From: Russell King - ARM Linux admin @ 2019-04-03 9:40 UTC (permalink / raw) To: Antoine Tenart Cc: Heiner Kallweit, davem, andrew, f.fainelli, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v4 2/2] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210 2019-04-02 13:10 [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks Antoine Tenart 2019-04-02 13:10 ` [PATCH net-next v4 1/2] " Antoine Tenart @ 2019-04-02 13:10 ` Antoine Tenart 2019-04-04 4:46 ` [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks David Miller 2 siblings, 0 replies; 9+ messages in thread From: Antoine Tenart @ 2019-04-02 13:10 UTC (permalink / raw) To: davem, linux, andrew, f.fainelli, hkallweit1 Cc: Antoine Tenart, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw When the 88x2110 PHY support was added, the suspend and resume callbacks were forgotten. This patch adds them to the 88x2110 PHY callback definition. Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/phy/marvell10g.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 9ee033c8a12b..9df6c2310f30 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -509,6 +509,8 @@ static struct phy_driver mv3310_drivers[] = { .name = "mv88x2110", .get_features = genphy_c45_pma_read_abilities, .probe = mv3310_probe, + .suspend = mv3310_suspend, + .resume = mv3310_resume, .soft_reset = genphy_no_soft_reset, .config_init = mv3310_config_init, .config_aneg = mv3310_config_aneg, -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks 2019-04-02 13:10 [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks Antoine Tenart 2019-04-02 13:10 ` [PATCH net-next v4 1/2] " Antoine Tenart 2019-04-02 13:10 ` [PATCH net-next v4 2/2] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210 Antoine Tenart @ 2019-04-04 4:46 ` David Miller 2 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2019-04-04 4:46 UTC (permalink / raw) To: antoine.tenart Cc: linux, andrew, f.fainelli, hkallweit1, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier, gregory.clement, miquel.raynal, nadavh, stefanc, mw From: Antoine Tenart <antoine.tenart@bootlin.com> Date: Tue, 2 Apr 2019 15:10:27 +0200 > This series implements the suspend/resume callbacks in the marvell10g > PHY driver. Series applied, thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-04-04 4:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-02 13:10 [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks Antoine Tenart 2019-04-02 13:10 ` [PATCH net-next v4 1/2] " Antoine Tenart 2019-04-02 18:17 ` Heiner Kallweit 2019-04-02 22:10 ` Russell King - ARM Linux admin 2019-04-03 5:09 ` Heiner Kallweit 2019-04-03 8:46 ` Antoine Tenart 2019-04-03 9:40 ` Russell King - ARM Linux admin 2019-04-02 13:10 ` [PATCH net-next v4 2/2] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210 Antoine Tenart 2019-04-04 4:46 ` [PATCH net-next v4 0/2] net: phy: marvell10g: implement suspend/resume callbacks David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.