When using temperature measurement on Marvell 88Q2XXX devices and the
reset-gpios property is set in DT, the device does a hardware reset when
interface is brought down and up again. That means that the content of
the register MDIO_MMD_PCS_MV_TEMP_SENSOR2 is reset to default and that
leads to permanent deactivation of the temperature measurement, because
activation is done in mv88q2xxx_probe. To fix this move activation of
temperature measurement to mv88q222x_config_init.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 4494b3e39ce2b672efe49d53d7021b765def6aa6..a3996471a1c9a5d4060d5d19ce44aa70e902a83f 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -95,6 +95,10 @@
 
 #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
 
+struct mv88q2xxx_priv {
+	bool enable_temp;
+};
+
 struct mmd_val {
 	int devad;
 	u32 regnum;
@@ -710,17 +714,12 @@ static const struct hwmon_chip_info mv88q2xxx_hwmon_chip_info = {
 
 static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
 {
+	struct mv88q2xxx_priv *priv = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	struct device *hwmon;
 	char *hwmon_name;
-	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;
 	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
 	if (IS_ERR(hwmon_name))
 		return PTR_ERR(hwmon_name);
@@ -743,6 +742,14 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
 
 static int mv88q2xxx_probe(struct phy_device *phydev)
 {
+	struct mv88q2xxx_priv *priv;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
 	return mv88q2xxx_hwmon_probe(phydev);
 }
 
@@ -810,6 +817,18 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
 
 static int mv88q222x_config_init(struct phy_device *phydev)
 {
+	struct mv88q2xxx_priv *priv = phydev->priv;
+	int ret;
+
+	/* Enable temperature sense */
+	if (priv->enable_temp) {
+		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;
+	}
+
 	if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0)
 		return mv88q222x_revb0_config_init(phydev);
 	else

---
base-commit: b44e27b4df1a1cd3fd84cf26c82156ed0301575f
change-id: 20250116-marvell-88q2xxx-fix-hwmon-d6700aae9227

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

>  static int mv88q222x_config_init(struct phy_device *phydev)
>  {
> +	struct mv88q2xxx_priv *priv = phydev->priv;
> +	int ret;
> +
> +	/* Enable temperature sense */
> +	if (priv->enable_temp) {
> +		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;
> +	}

Does enabling the sensor when it is already enabled cause issues? I'm
not sure if it is worth having priv->enable_temp just to save one
write which is going to be performed very infrequently.

	Andrew

Hi Andrew,

Am Fri, Jan 17, 2025 at 02:22:55PM +0100 schrieb Andrew Lunn:
> >  static int mv88q222x_config_init(struct phy_device *phydev)
> >  {
> > +	struct mv88q2xxx_priv *priv = phydev->priv;
> > +	int ret;
> > +
> > +	/* Enable temperature sense */
> > +	if (priv->enable_temp) {
> > +		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;
> > +	}
> 
> Does enabling the sensor when it is already enabled cause issues? I'm
> not sure if it is worth having priv->enable_temp just to save one
> write which is going to be performed very infrequently.

Tested it, there haven't been any issues with enabling it again.

You are right, but I would need struct mv88q2xxx_priv anyway for patch:
https://lore.kernel.org/netdev/20250110-marvell-88q2xxx-leds-v1-1-22e7734941c2@gmail.com/

There I'm running into the same issue as here and have to fix it there
too. Then it makes sense to keep the priv->enable_temp. I just wanted to
fix this issue before I continue with the patch for LED driver.

By the way, I forgot to add the fixes tag:
Fixes: a557a92e6881 ("net: phy: marvell-88q2xxx: add support for temperature sensor")
Will add it in V2.

Best regards,
Dimitri

Hello Dimitri,

Thanks for your work.

On 2025-01-16 16:37:44 +0100, Dimitri Fedrau wrote:
> When using temperature measurement on Marvell 88Q2XXX devices and the
> reset-gpios property is set in DT, the device does a hardware reset when
> interface is brought down and up again. That means that the content of
> the register MDIO_MMD_PCS_MV_TEMP_SENSOR2 is reset to default and that
> leads to permanent deactivation of the temperature measurement, because
> activation is done in mv88q2xxx_probe. To fix this move activation of
> temperature measurement to mv88q222x_config_init.

I only have access to a mv88q2110 device so I can't test the change, but 
it looks good.

> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

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

> ---
>  drivers/net/phy/marvell-88q2xxx.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 4494b3e39ce2b672efe49d53d7021b765def6aa6..a3996471a1c9a5d4060d5d19ce44aa70e902a83f 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -95,6 +95,10 @@
>  
>  #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
>  
> +struct mv88q2xxx_priv {
> +	bool enable_temp;
> +};
> +
>  struct mmd_val {
>  	int devad;
>  	u32 regnum;
> @@ -710,17 +714,12 @@ static const struct hwmon_chip_info mv88q2xxx_hwmon_chip_info = {
>  
>  static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>  {
> +	struct mv88q2xxx_priv *priv = phydev->priv;
>  	struct device *dev = &phydev->mdio.dev;
>  	struct device *hwmon;
>  	char *hwmon_name;
> -	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;
>  	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
>  	if (IS_ERR(hwmon_name))
>  		return PTR_ERR(hwmon_name);
> @@ -743,6 +742,14 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>  
>  static int mv88q2xxx_probe(struct phy_device *phydev)
>  {
> +	struct mv88q2xxx_priv *priv;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phydev->priv = priv;
> +
>  	return mv88q2xxx_hwmon_probe(phydev);
>  }
>  
> @@ -810,6 +817,18 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
>  
>  static int mv88q222x_config_init(struct phy_device *phydev)
>  {
> +	struct mv88q2xxx_priv *priv = phydev->priv;
> +	int ret;
> +
> +	/* Enable temperature sense */
> +	if (priv->enable_temp) {
> +		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;
> +	}
> +
>  	if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0)
>  		return mv88q222x_revb0_config_init(phydev);
>  	else
> 
> ---
> base-commit: b44e27b4df1a1cd3fd84cf26c82156ed0301575f
> change-id: 20250116-marvell-88q2xxx-fix-hwmon-d6700aae9227
> 
> Best regards,
> -- 
> Dimitri Fedrau <dima.fedrau@gmail.com>
> 

-- 
Kind Regards,
Niklas Söderlund