[v3] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[PATCH net-next v3] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

Posted by Dimitri Fedrau 9 months, 2 weeks ago

Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
mv88q222x_config_init with the consequence that the sensor is only
usable when the PHY is configured. Enable the sensor in
mv88q2xxx_hwmon_probe as well to fix this.

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
Changes in v3:
- Remove patch "net: phy: marvell-88q2xxx: Prevent hwmon access with asserted reset"
  from series. There will be a separate patch handling this and I'm not
  sure if it is going to be accepted. Separating this is necessary
  because the temperature reading is somehow odd at the moment, because
  the interface has to be brought up for it to work. See:
  https://lore.kernel.org/netdev/20250418145800.2420751-1-niklas.soderlund+renesas@ragnatech.se/
- Link to v2: https://lore.kernel.org/r/20250220-marvell-88q2xxx-hwmon-enable-at-probe-v2-0-78b2838a62da@gmail.com

Changes in v2:
- Add comment in mv88q2xxx_config_init why the temperature sensor is
  enabled again (Stefan)
- Fix commit message by adding the information why the PHY reset might
  be asserted. (Andrew)
- Remove fixes tags (Andrew)
- Switch to net-next (Andrew)
- Return ENETDOWN instead of EIO when PHYs reset is asserted in
  mv88q2xxx_hwmon_read (Andrew)
- Add check if PHYs reset is asserted in mv88q2xxx_hwmon_write as it was
  done in mv88q2xxx_hwmon_read
- Link to v1: https://lore.kernel.org/r/20250218-marvell-88q2xxx-hwmon-enable-at-probe-v1-0-999a304c8a11@gmail.com
---
 drivers/net/phy/marvell-88q2xxx.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 5c687164b8e068f3f09e91cd4dd198f24782682e..5d2fbbf332933ffe06f4506058e380fbc7c52921 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -513,7 +513,10 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
 			return ret;
 	}
 
-	/* Enable temperature sense */
+	/* Enable temperature sense again. There might have been a hard reset
+	 * of the PHY and in this case the register content is restored to
+	 * defaults and we need to enable it again.
+	 */
 	if (priv->enable_temp) {
 		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
 				     MDIO_MMD_PCS_MV_TEMP_SENSOR2,
@@ -765,6 +768,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
 	struct mv88q2xxx_priv *priv = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	struct device *hwmon;
+	int ret;
+
+	/* Enable temperature sense */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
+			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
+	if (ret < 0)
+		return ret;
 
 	priv->enable_temp = true;
 

---
base-commit: 0d15a26b247d25cd012134bf8825128fedb15cc9
change-id: 20250217-marvell-88q2xxx-hwmon-enable-at-probe-2a2666325985

Best regards,
-- 
Dimitri Fedrau <dima.fedrau@gmail.com>

Re: [PATCH net-next v3] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

Posted by Niklas Söderlund 9 months, 2 weeks ago

Hi Dimitri,

Thanks for your work.

On 2025-04-29 08:54:25 +0200, Dimitri Fedrau wrote:
> Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
> mv88q222x_config_init with the consequence that the sensor is only
> usable when the PHY is configured. Enable the sensor in
> mv88q2xxx_hwmon_probe as well to fix this.
> 
> Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
> Changes in v3:
> - Remove patch "net: phy: marvell-88q2xxx: Prevent hwmon access with asserted reset"
>   from series. There will be a separate patch handling this and I'm not
>   sure if it is going to be accepted. Separating this is necessary
>   because the temperature reading is somehow odd at the moment, because
>   the interface has to be brought up for it to work. See:
>   https://lore.kernel.org/netdev/20250418145800.2420751-1-niklas.soderlund+renesas@ragnatech.se/
> - Link to v2: https://lore.kernel.org/r/20250220-marvell-88q2xxx-hwmon-enable-at-probe-v2-0-78b2838a62da@gmail.com
> 
> Changes in v2:
> - Add comment in mv88q2xxx_config_init why the temperature sensor is
>   enabled again (Stefan)
> - Fix commit message by adding the information why the PHY reset might
>   be asserted. (Andrew)
> - Remove fixes tags (Andrew)
> - Switch to net-next (Andrew)
> - Return ENETDOWN instead of EIO when PHYs reset is asserted in
>   mv88q2xxx_hwmon_read (Andrew)
> - Add check if PHYs reset is asserted in mv88q2xxx_hwmon_write as it was
>   done in mv88q2xxx_hwmon_read
> - Link to v1: https://lore.kernel.org/r/20250218-marvell-88q2xxx-hwmon-enable-at-probe-v1-0-999a304c8a11@gmail.com
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 5c687164b8e068f3f09e91cd4dd198f24782682e..5d2fbbf332933ffe06f4506058e380fbc7c52921 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -513,7 +513,10 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
>  			return ret;
>  	}
>  
> -	/* Enable temperature sense */
> +	/* Enable temperature sense again. There might have been a hard reset
> +	 * of the PHY and in this case the register content is restored to
> +	 * defaults and we need to enable it again.
> +	 */
>  	if (priv->enable_temp) {
>  		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
>  				     MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> @@ -765,6 +768,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>  	struct mv88q2xxx_priv *priv = phydev->priv;
>  	struct device *dev = &phydev->mdio.dev;
>  	struct device *hwmon;
> +	int ret;
> +
> +	/* Enable temperature sense */
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> +	if (ret < 0)
> +		return ret;

nit: I wonder if it make sens to create a helper function to enable the 
sensor? My worry being this procedure growing in the future and only 
being fixed in one location and not the other. It would also reduce code 
duplication and could be stubbed to be compiled out with the existing 
IS_ENABLED(CONFIG_HWMON) guard for other hwmon functions.

That being said I tested this with mv88q211x and the temp sensor and PHY 
keeps working as expected.

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

>  
>  	priv->enable_temp = true;
>  
> 
> ---
> base-commit: 0d15a26b247d25cd012134bf8825128fedb15cc9
> change-id: 20250217-marvell-88q2xxx-hwmon-enable-at-probe-2a2666325985
> 
> Best regards,
> -- 
> Dimitri Fedrau <dima.fedrau@gmail.com>
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH net-next v3] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

Posted by Dimitri Fedrau 9 months, 1 week ago

Hi Niklas,

Am Tue, Apr 29, 2025 at 10:03:06PM +0200 schrieb Niklas Söderlund:
> Hi Dimitri,
> 
> Thanks for your work.
> 
> On 2025-04-29 08:54:25 +0200, Dimitri Fedrau wrote:
> > Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
> > mv88q222x_config_init with the consequence that the sensor is only
> > usable when the PHY is configured. Enable the sensor in
> > mv88q2xxx_hwmon_probe as well to fix this.
> > 
> > Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> > ---
> > Changes in v3:
> > - Remove patch "net: phy: marvell-88q2xxx: Prevent hwmon access with asserted reset"
> >   from series. There will be a separate patch handling this and I'm not
> >   sure if it is going to be accepted. Separating this is necessary
> >   because the temperature reading is somehow odd at the moment, because
> >   the interface has to be brought up for it to work. See:
> >   https://lore.kernel.org/netdev/20250418145800.2420751-1-niklas.soderlund+renesas@ragnatech.se/
> > - Link to v2: https://lore.kernel.org/r/20250220-marvell-88q2xxx-hwmon-enable-at-probe-v2-0-78b2838a62da@gmail.com
> > 
> > Changes in v2:
> > - Add comment in mv88q2xxx_config_init why the temperature sensor is
> >   enabled again (Stefan)
> > - Fix commit message by adding the information why the PHY reset might
> >   be asserted. (Andrew)
> > - Remove fixes tags (Andrew)
> > - Switch to net-next (Andrew)
> > - Return ENETDOWN instead of EIO when PHYs reset is asserted in
> >   mv88q2xxx_hwmon_read (Andrew)
> > - Add check if PHYs reset is asserted in mv88q2xxx_hwmon_write as it was
> >   done in mv88q2xxx_hwmon_read
> > - Link to v1: https://lore.kernel.org/r/20250218-marvell-88q2xxx-hwmon-enable-at-probe-v1-0-999a304c8a11@gmail.com
> > ---
> >  drivers/net/phy/marvell-88q2xxx.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> > index 5c687164b8e068f3f09e91cd4dd198f24782682e..5d2fbbf332933ffe06f4506058e380fbc7c52921 100644
> > --- a/drivers/net/phy/marvell-88q2xxx.c
> > +++ b/drivers/net/phy/marvell-88q2xxx.c
> > @@ -513,7 +513,10 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
> >  			return ret;
> >  	}
> >  
> > -	/* Enable temperature sense */
> > +	/* Enable temperature sense again. There might have been a hard reset
> > +	 * of the PHY and in this case the register content is restored to
> > +	 * defaults and we need to enable it again.
> > +	 */
> >  	if (priv->enable_temp) {
> >  		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> >  				     MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> > @@ -765,6 +768,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> >  	struct mv88q2xxx_priv *priv = phydev->priv;
> >  	struct device *dev = &phydev->mdio.dev;
> >  	struct device *hwmon;
> > +	int ret;
> > +
> > +	/* Enable temperature sense */
> > +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> > +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> > +	if (ret < 0)
> > +		return ret;
> 
> nit: I wonder if it make sens to create a helper function to enable the 
> sensor? My worry being this procedure growing in the future and only 
> being fixed in one location and not the other. It would also reduce code 
> duplication and could be stubbed to be compiled out with the existing 
> IS_ENABLED(CONFIG_HWMON) guard for other hwmon functions.
>

thanks for testing. Wouldn't it then be better to implement HWMON_T_ENABLE
instead ? Add the enablement into mv88q2xxx_hwmon_write and the user is
responsible for enablement of the sensor ?

Best regards,
Dimitri Fedrau

Re: [PATCH net-next v3] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

Posted by Paolo Abeni 9 months, 1 week ago

On 4/29/25 10:03 PM, Niklas Söderlund wrote:
>> @@ -765,6 +768,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>>  	struct mv88q2xxx_priv *priv = phydev->priv;
>>  	struct device *dev = &phydev->mdio.dev;
>>  	struct device *hwmon;
>> +	int ret;
>> +
>> +	/* Enable temperature sense */
>> +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
>> +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
>> +	if (ret < 0)
>> +		return ret;
> 
> nit: I wonder if it make sens to create a helper function to enable the 
> sensor? My worry being this procedure growing in the future and only 
> being fixed in one location and not the other. It would also reduce code 
> duplication and could be stubbed to be compiled out with the existing 
> IS_ENABLED(CONFIG_HWMON) guard for other hwmon functions.
> 
> That being said I tested this with mv88q211x and the temp sensor and PHY 
> keeps working as expected.
> 
> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Since this is net-next material +1 for the helper.

Also AFAICS this is fixing a net-next regression, so it needs a fixes
tag, too.

Thanks,

Paolo

Re: [PATCH net-next v3] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

Posted by Dimitri Fedrau 9 months, 1 week ago

Hi Paolo,

On Fri, May 02, 2025 at 12:51:47PM +0200, Paolo Abeni wrote:
> On 4/29/25 10:03 PM, Niklas Söderlund wrote:
> >> @@ -765,6 +768,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> >>  	struct mv88q2xxx_priv *priv = phydev->priv;
> >>  	struct device *dev = &phydev->mdio.dev;
> >>  	struct device *hwmon;
> >> +	int ret;
> >> +
> >> +	/* Enable temperature sense */
> >> +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> >> +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> >> +	if (ret < 0)
> >> +		return ret;
> > 
> > nit: I wonder if it make sens to create a helper function to enable the 
> > sensor? My worry being this procedure growing in the future and only 
> > being fixed in one location and not the other. It would also reduce code 
> > duplication and could be stubbed to be compiled out with the existing 
> > IS_ENABLED(CONFIG_HWMON) guard for other hwmon functions.
> > 
> > That being said I tested this with mv88q211x and the temp sensor and PHY 
> > keeps working as expected.
> > 
> > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Since this is net-next material +1 for the helper.
>
Agreed.

> Also AFAICS this is fixing a net-next regression, so it needs a fixes
> tag, too.
> 
We discussed it here:
https://lore.kernel.org/netdev/48c4cd14-be56-438e-9561-c85b0245178c@lunn.ch/

Andrews opinion was the change is rather archtitecture then a fix.

Best regards,
Dimitri Fedrau