| Message ID | 20250218-marvell-88q2xxx-hwmon-enable-at-probe-v1-1-999a304c8a11@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell-88q2xxx: Enable temperature measurement in probe again | expand |
Hi Dimitri, On Tue, Feb 18, 2025 at 07:33:09PM +0100, 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. > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > --- > drivers/net/phy/marvell-88q2xxx.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644 > --- a/drivers/net/phy/marvell-88q2xxx.c > +++ b/drivers/net/phy/marvell-88q2xxx.c > @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) > 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)); Is it necessary to have it enabled in probe and in config? Is that because of the soft reset? Can it happen that the phy is reset but config is not called, then we would end up in the same situation right? Regards, Stefan
Hi Stefan, thanks for reviewing. Am Wed, Feb 19, 2025 at 07:16:58AM +0100 schrieb Stefan Eichenberger: > Hi Dimitri, > > On Tue, Feb 18, 2025 at 07:33:09PM +0100, 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. > > > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > --- > > drivers/net/phy/marvell-88q2xxx.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644 > > --- a/drivers/net/phy/marvell-88q2xxx.c > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) > > 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)); > > Is it necessary to have it enabled in probe and in config? Is that > because of the soft reset? Can it happen that the phy is reset but > config is not called, then we would end up in the same situation right? > Even if the phy is not configured yet, it is probed and the PHYs hard reset is deasserted, so I can read the temperature. I think the situation you mean is when the PHY is brought up and down again. In this case the hard reset of the PHY is asserted and I can't read the temperature. That's the second patch of the series that fixes this issue. Best regards, Dimitri
Hi Dimitri, On Wed, Feb 19, 2025 at 11:46:47AM +0100, Dimitri Fedrau wrote: > Hi Stefan, > > thanks for reviewing. > > Am Wed, Feb 19, 2025 at 07:16:58AM +0100 schrieb Stefan Eichenberger: > > Hi Dimitri, > > > > On Tue, Feb 18, 2025 at 07:33:09PM +0100, 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. > > > > > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > --- > > > drivers/net/phy/marvell-88q2xxx.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644 > > > --- a/drivers/net/phy/marvell-88q2xxx.c > > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > > @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) > > > 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)); > > > > Is it necessary to have it enabled in probe and in config? Is that > > because of the soft reset? Can it happen that the phy is reset but > > config is not called, then we would end up in the same situation right? > > > Even if the phy is not configured yet, it is probed and the PHYs hard reset > is deasserted, so I can read the temperature. I think the situation you > mean is when the PHY is brought up and down again. In this case the hard > reset of the PHY is asserted and I can't read the temperature. That's > the second patch of the series that fixes this issue. Okay I see, thanks for the explaination. So the code in mv88q222x_config_init is required after a hard reset of the phy. I was just thinking, if we could avoid the duplication but I guess both enables are required. Could you maybe add a comment why we need to have it enabled in both places? Regards, Stefan
Am Wed, Feb 19, 2025 at 06:25:47PM +0100 schrieb Stefan Eichenberger: > Hi Dimitri, > > On Wed, Feb 19, 2025 at 11:46:47AM +0100, Dimitri Fedrau wrote: > > Hi Stefan, > > > > thanks for reviewing. > > > > Am Wed, Feb 19, 2025 at 07:16:58AM +0100 schrieb Stefan Eichenberger: > > > Hi Dimitri, > > > > > > On Tue, Feb 18, 2025 at 07:33:09PM +0100, 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. > > > > > > > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > > --- > > > > drivers/net/phy/marvell-88q2xxx.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > > > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644 > > > > --- a/drivers/net/phy/marvell-88q2xxx.c > > > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > > > @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) > > > > 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)); > > > > > > Is it necessary to have it enabled in probe and in config? Is that > > > because of the soft reset? Can it happen that the phy is reset but > > > config is not called, then we would end up in the same situation right? > > > > > Even if the phy is not configured yet, it is probed and the PHYs hard reset > > is deasserted, so I can read the temperature. I think the situation you > > mean is when the PHY is brought up and down again. In this case the hard > > reset of the PHY is asserted and I can't read the temperature. That's > > the second patch of the series that fixes this issue. > > Okay I see, thanks for the explaination. So the code in > mv88q222x_config_init is required after a hard reset of the phy. I was > just thinking, if we could avoid the duplication but I guess both > enables are required. Could you maybe add a comment why we need to have > it enabled in both places? > Will add the comment.
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644 --- a/drivers/net/phy/marvell-88q2xxx.c +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) 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));
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. Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> --- drivers/net/phy/marvell-88q2xxx.c | 7 +++++++ 1 file changed, 7 insertions(+)