Marvell 88q2xxx devices have an inbuilt temperature sensor. Add hwmon
support for this sensor.
Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
drivers/net/phy/marvell-88q2xxx.c | 152 ++++++++++++++++++++++++++++++
1 file changed, 152 insertions(+)
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 4cb8fe524795..6900bad275d0 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -5,6 +5,7 @@
#include <linux/ethtool_netlink.h>
#include <linux/marvell_phy.h>
#include <linux/phy.h>
+#include <linux/hwmon.h>
#define PHY_ID_88Q2220_REVB0 (MARVELL_PHY_ID_88Q2220 | 0x1)
@@ -33,6 +34,19 @@
#define MDIO_MMD_PCS_MV_GPIO_INT_CTRL 32787
#define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS 0x0800
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1 32833
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT 0x0001
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT 0x0040
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN 0x0080
+
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR2 32834
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK 0xc000
+
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3 32835
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK 0xff00
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT 8
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK 0x00ff
+
#define MDIO_MMD_PCS_MV_100BT1_STAT1 33032
#define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR 0x00ff
#define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER 0x0100
@@ -488,6 +502,143 @@ static int mv88q2xxx_resume(struct phy_device *phydev)
return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
MDIO_CTRL1_LPOWER);
}
+#ifdef CONFIG_HWMON
+static const struct hwmon_channel_info * const mv88q2xxx_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_ALARM),
+ NULL
+};
+
+static umode_t mv88q2xxx_hwmon_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (attr) {
+ case hwmon_temp_input:
+ return 0444;
+ case hwmon_temp_max:
+ return 0644;
+ case hwmon_temp_alarm:
+ return 0444;
+ default:
+ return 0;
+ }
+}
+
+static int mv88q2xxx_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct phy_device *phydev = dev_get_drvdata(dev);
+ int ret;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR3);
+ if (ret < 0)
+ return ret;
+
+ *val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;
+ return 0;
+ case hwmon_temp_max:
+ ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR3);
+ if (ret < 0)
+ return ret;
+
+ *val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
+ MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
+ 1000;
+ return 0;
+ case hwmon_temp_alarm:
+ ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR1);
+ if (ret < 0)
+ return ret;
+
+ *val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int mv88q2xxx_hwmon_write(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long val)
+{
+ struct phy_device *phydev = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_temp_max:
+ if (val < -75000 || val > 180000)
+ return -EINVAL;
+
+ val = ((val / 1000) + 75) <<
+ MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;
+ return phy_modify_mmd(phydev, MDIO_MMD_PCS,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR3,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
+ val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static const struct hwmon_ops mv88q2xxx_hwmon_hwmon_ops = {
+ .is_visible = mv88q2xxx_hwmon_is_visible,
+ .read = mv88q2xxx_hwmon_read,
+ .write = mv88q2xxx_hwmon_write,
+};
+
+static const struct hwmon_chip_info mv88q2xxx_hwmon_chip_info = {
+ .ops = &mv88q2xxx_hwmon_hwmon_ops,
+ .info = mv88q2xxx_hwmon_info,
+};
+
+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 sensor interrupt */
+ ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR1,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
+ if (ret < 0)
+ return 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;
+
+ hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
+ if (IS_ERR(hwmon_name))
+ return PTR_ERR(hwmon_name);
+
+ hwmon = devm_hwmon_device_register_with_info(dev,
+ hwmon_name,
+ phydev,
+ &mv88q2xxx_hwmon_chip_info,
+ NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon);
+}
+
+#else
+static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
+{
+ return 0;
+}
+#endif
+static int mv88q2xxx_probe(struct phy_device *phydev)
+{
+ return mv88q2xxx_hwmon_probe(phydev);
+}
static int mv88q222x_soft_reset(struct phy_device *phydev)
{
@@ -583,6 +734,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
{
PHY_ID_MATCH_EXACT(PHY_ID_88Q2220_REVB0),
.name = "mv88q2220",
+ .probe = mv88q2xxx_probe,
.get_features = mv88q2xxx_get_features,
.config_aneg = mv88q222x_config_aneg,
.aneg_done = genphy_c45_aneg_done,
--
2.39.2On 1/22/24 13:28, Dimitri Fedrau wrote:
> Marvell 88q2xxx devices have an inbuilt temperature sensor. Add hwmon
> support for this sensor.
>
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
> drivers/net/phy/marvell-88q2xxx.c | 152 ++++++++++++++++++++++++++++++
> 1 file changed, 152 insertions(+)
>
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 4cb8fe524795..6900bad275d0 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -5,6 +5,7 @@
> #include <linux/ethtool_netlink.h>
> #include <linux/marvell_phy.h>
> #include <linux/phy.h>
> +#include <linux/hwmon.h>
>
> #define PHY_ID_88Q2220_REVB0 (MARVELL_PHY_ID_88Q2220 | 0x1)
>
> @@ -33,6 +34,19 @@
> #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL 32787
> #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS 0x0800
>
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1 32833
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT 0x0001
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT 0x0040
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN 0x0080
> +
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR2 32834
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK 0xc000
> +
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3 32835
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK 0xff00
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT 8
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK 0x00ff
> +
> #define MDIO_MMD_PCS_MV_100BT1_STAT1 33032
> #define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR 0x00ff
> #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER 0x0100
> @@ -488,6 +502,143 @@ static int mv88q2xxx_resume(struct phy_device *phydev)
> return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
> MDIO_CTRL1_LPOWER);
> }
> +#ifdef CONFIG_HWMON
HWMON is tristate, so this may be problematic if the driver is built
into the kernel and hwmon is built as module.
[ ... ]
> +
> +static int mv88q2xxx_hwmon_write(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, long val)
> +{
> + struct phy_device *phydev = dev_get_drvdata(dev);
> +
> + switch (attr) {
> + case hwmon_temp_max:
> + if (val < -75000 || val > 180000)
> + return -EINVAL;
> +
Not that it matters much, but we typically use clamp_val() to limit
the range of temperature limits because the valid range differs for
each chip and is otherwise difficult to determine for the user.
Guenter
Am Thu, Feb 01, 2024 at 05:18:23AM -0800 schrieb Guenter Roeck:
[...]
> > +
> > +static int mv88q2xxx_hwmon_write(struct device *dev,
> > + enum hwmon_sensor_types type, u32 attr,
> > + int channel, long val)
> > +{
> > + struct phy_device *phydev = dev_get_drvdata(dev);
> > +
> > + switch (attr) {
> > + case hwmon_temp_max:
> > + if (val < -75000 || val > 180000)
> > + return -EINVAL;
> > +
>
> Not that it matters much, but we typically use clamp_val() to limit
> the range of temperature limits because the valid range differs for
> each chip and is otherwise difficult to determine for the user.
>
Will fix it.
Dimitri
> > +#ifdef CONFIG_HWMON
>
> HWMON is tristate, so this may be problematic if the driver is built
> into the kernel and hwmon is built as module.
There should be Kconfig in addition to this, e.g.
config MAXLINEAR_GPHY
tristate "Maxlinear Ethernet PHYs"
select POLYNOMIAL if HWMON
depends on HWMON || HWMON=n
help
Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
GPY241, GPY245 PHYs.
So its forced to being built in, or not built at all.
Andrew
On 2/1/24 05:27, Andrew Lunn wrote: >>> +#ifdef CONFIG_HWMON >> >> HWMON is tristate, so this may be problematic if the driver is built >> into the kernel and hwmon is built as module. > > There should be Kconfig in addition to this, e.g. > > config MAXLINEAR_GPHY > tristate "Maxlinear Ethernet PHYs" > select POLYNOMIAL if HWMON > depends on HWMON || HWMON=n > help > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215, > GPY241, GPY245 PHYs. > > So its forced to being built in, or not built at all. > Even then it should be "#if IS_ENABLED(HWMON)" in the code. Thanks, Guenter
Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck: > On 2/1/24 05:27, Andrew Lunn wrote: > > > > +#ifdef CONFIG_HWMON > > > > > > HWMON is tristate, so this may be problematic if the driver is built > > > into the kernel and hwmon is built as module. > > > > There should be Kconfig in addition to this, e.g. > > > > config MAXLINEAR_GPHY > > tristate "Maxlinear Ethernet PHYs" > > select POLYNOMIAL if HWMON > > depends on HWMON || HWMON=n > > help > > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215, > > GPY241, GPY245 PHYs. > > > > So its forced to being built in, or not built at all. > > > > Even then it should be "#if IS_ENABLED(HWMON)" in the code. > > If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in the KConfig file ? When looking at other PHY drivers, they do. Dimitri
On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > +#ifdef CONFIG_HWMON
> > > >
> > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > into the kernel and hwmon is built as module.
> > >
> > > There should be Kconfig in addition to this, e.g.
> > >
> > > config MAXLINEAR_GPHY
> > > tristate "Maxlinear Ethernet PHYs"
> > > select POLYNOMIAL if HWMON
> > > depends on HWMON || HWMON=n
> > > help
> > > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > > GPY241, GPY245 PHYs.
> > >
> > > So its forced to being built in, or not built at all.
> > >
> >
> > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> >
> >
> If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> the KConfig file ? When looking at other PHY drivers, they do.
Yes, to handle CONFIG_HWMON=m. Note that it is "IS_ENABLED(CONFIG_HWMON)"
^^^^^^^
Guenter
On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote: > Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck: > > On 2/1/24 05:27, Andrew Lunn wrote: > > > > > +#ifdef CONFIG_HWMON > > > > > > > > HWMON is tristate, so this may be problematic if the driver is built > > > > into the kernel and hwmon is built as module. > > > > > > There should be Kconfig in addition to this, e.g. > > > > > > config MAXLINEAR_GPHY > > > tristate "Maxlinear Ethernet PHYs" > > > select POLYNOMIAL if HWMON > > > depends on HWMON || HWMON=n > > > help > > > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215, > > > GPY241, GPY245 PHYs. > > > > > > So its forced to being built in, or not built at all. > > > > > > > Even then it should be "#if IS_ENABLED(HWMON)" in the code. > > > > > If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in > the KConfig file ? When looking at other PHY drivers, they do. Please follow what other drivers do. Its easy to break the build, resulting is undefined symbols. What we have now works. Andrew
Am Thu, Feb 01, 2024 at 05:47:21PM +0100 schrieb Andrew Lunn: > On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote: > > Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck: > > > On 2/1/24 05:27, Andrew Lunn wrote: > > > > > > +#ifdef CONFIG_HWMON > > > > > > > > > > HWMON is tristate, so this may be problematic if the driver is built > > > > > into the kernel and hwmon is built as module. > > > > > > > > There should be Kconfig in addition to this, e.g. > > > > > > > > config MAXLINEAR_GPHY > > > > tristate "Maxlinear Ethernet PHYs" > > > > select POLYNOMIAL if HWMON > > > > depends on HWMON || HWMON=n > > > > help > > > > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215, > > > > GPY241, GPY245 PHYs. > > > > > > > > So its forced to being built in, or not built at all. > > > > > > > > > > Even then it should be "#if IS_ENABLED(HWMON)" in the code. > > > > > > > > If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in > > the KConfig file ? When looking at other PHY drivers, they do. > > Please follow what other drivers do. Its easy to break the build, > resulting is undefined symbols. What we have now works. Sure.
> +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 sensor interrupt */
> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> + MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> + MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
You enable an interrupt, but i don't see any changes to the interrupt
handler to handle any interrupts which are generated?
Andrew
Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > +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 sensor interrupt */
> > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > + MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > + MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
>
> You enable an interrupt, but i don't see any changes to the interrupt
> handler to handle any interrupts which are generated?
>
Hi Andrew,
you are right. Have to remove these lines. Besides enabling the interrupt
in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
necessary to make the interrupt propagate. I didn't want it to propagate.
Anyway it's wrong. I couldn't find a good solution to use the temperature
interrupt. Will have a look into this, and probably figuring out how to
do so. But it won't be part of this patch series.
Dimitri
On 1/31/24 23:11, Dimitri Fedrau wrote:
> Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
>>> +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 sensor interrupt */
>>> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
>>> + MDIO_MMD_PCS_MV_TEMP_SENSOR1,
>>> + MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
>>
>> You enable an interrupt, but i don't see any changes to the interrupt
>> handler to handle any interrupts which are generated?
>>
> Hi Andrew,
>
> you are right. Have to remove these lines. Besides enabling the interrupt
> in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> necessary to make the interrupt propagate. I didn't want it to propagate.
> Anyway it's wrong. I couldn't find a good solution to use the temperature
> interrupt. Will have a look into this, and probably figuring out how to
> do so. But it won't be part of this patch series.
>
From hwmon perspective, the expected use of such an interrupt would be
to call hwmon_notify_event() with the affected limit attribute as argument.
This would notify the thermal subsystem if the sensor is registered with it
(your patch doesn't set the necessary flag when registering the driver,
so this would not happen), it will send a notification to the sysfs
attribute, and generate a udev event.
Guenter
Am Thu, Feb 01, 2024 at 05:34:05AM -0800 schrieb Guenter Roeck:
> On 1/31/24 23:11, Dimitri Fedrau wrote:
> > Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > > > +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 sensor interrupt */
> > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > > > + MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > > > + MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> > >
> > > You enable an interrupt, but i don't see any changes to the interrupt
> > > handler to handle any interrupts which are generated?
> > >
> > Hi Andrew,
> >
> > you are right. Have to remove these lines. Besides enabling the interrupt
> > in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> > necessary to make the interrupt propagate. I didn't want it to propagate.
> > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > interrupt. Will have a look into this, and probably figuring out how to
> > do so. But it won't be part of this patch series.
> >
>
> From hwmon perspective, the expected use of such an interrupt would be
> to call hwmon_notify_event() with the affected limit attribute as argument.
> This would notify the thermal subsystem if the sensor is registered with it
> (your patch doesn't set the necessary flag when registering the driver,
> so this would not happen), it will send a notification to the sysfs
> attribute, and generate a udev event.
>
Thanks, noted it down. Didn't know about the notification to the thermal
subsystem and the generated udev event. :)
Dimitri
On Thu, Feb 01, 2024 at 05:14:35PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:34:05AM -0800 schrieb Guenter Roeck:
> > On 1/31/24 23:11, Dimitri Fedrau wrote:
> > > Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > > > > +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 sensor interrupt */
> > > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > > > > + MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > > > > + MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> > > >
> > > > You enable an interrupt, but i don't see any changes to the interrupt
> > > > handler to handle any interrupts which are generated?
> > > >
> > > Hi Andrew,
> > >
> > > you are right. Have to remove these lines. Besides enabling the interrupt
> > > in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> > > necessary to make the interrupt propagate. I didn't want it to propagate.
> > > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > > interrupt. Will have a look into this, and probably figuring out how to
> > > do so. But it won't be part of this patch series.
> > >
> >
> > From hwmon perspective, the expected use of such an interrupt would be
> > to call hwmon_notify_event() with the affected limit attribute as argument.
> > This would notify the thermal subsystem if the sensor is registered with it
> > (your patch doesn't set the necessary flag when registering the driver,
> > so this would not happen), it will send a notification to the sysfs
> > attribute, and generate a udev event.
> >
> Thanks, noted it down. Didn't know about the notification to the thermal
> subsystem and the generated udev event. :)
>
Note that you'd have to add something like
HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
to the code to register the sensor as thermal zone.
Guenter
> Anyway it's wrong. I couldn't find a good solution to use the temperature > interrupt. Will have a look into this, and probably figuring out how to > do so. But it won't be part of this patch series. I don't know of any PHY driver you can follow, those that do have a temperature sensor just report the temperature and don't do anything in addition. You might need to look at thermal zones, and indicate there has been a thermal trip point. That could then be used by the thermal subsystem to increase cooling via a fan, etc. In theory, you could also make the PHY active to thermal pressure, by forcing the link to renegotiate to a lower link speed. If you decide to go this route, please try to make is generic to any PHY. But its going to be quite a disruptive thing, the link will be lost of a little over a second... Andrew
Am Thu, Feb 01, 2024 at 02:23:15PM +0100 schrieb Andrew Lunn: > > Anyway it's wrong. I couldn't find a good solution to use the temperature > > interrupt. Will have a look into this, and probably figuring out how to > > do so. But it won't be part of this patch series. > > I don't know of any PHY driver you can follow, those that do have a > temperature sensor just report the temperature and don't do anything > in addition. > > You might need to look at thermal zones, and indicate there has been a > thermal trip point. That could then be used by the thermal subsystem > to increase cooling via a fan, etc. In theory, you could also make the > PHY active to thermal pressure, by forcing the link to renegotiate to > a lower link speed. If you decide to go this route, please try to make > is generic to any PHY. But its going to be quite a disruptive thing, > the link will be lost of a little over a second... > Making the PHY active to thermal pressure sounds interesting. Will look into this.
On Mon, Jan 22, 2024 at 10:28:41PM +0100, Dimitri Fedrau wrote:
int tmp;
> + switch (attr) {
> + case hwmon_temp_input:
> + ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> + MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> + if (ret < 0)
> + return ret;
> +
> + *val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;
tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK, ret);
*val = (tmp - 75) * 1000;
> + return 0;
> + case hwmon_temp_max:
> + ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> + MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> + if (ret < 0)
> + return ret;
> +
> + *val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
> + MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
> + 1000;
tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
ret);
*val = (tmp - 75) * 1000;
> + return 0;
> + case hwmon_temp_alarm:
> + ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> + MDIO_MMD_PCS_MV_TEMP_SENSOR1);
> + if (ret < 0)
> + return ret;
> +
> + *val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int mv88q2xxx_hwmon_write(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, long val)
> +{
> + struct phy_device *phydev = dev_get_drvdata(dev);
> +
> + switch (attr) {
> + case hwmon_temp_max:
> + if (val < -75000 || val > 180000)
> + return -EINVAL;
> +
> + val = ((val / 1000) + 75) <<
> + MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;
val = (val / 1000) + 75;
val = FIELD_PREP(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
val);
... and therefore no need for the _SHIFT constants.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Am Tue, Jan 30, 2024 at 09:18:31AM +0000 schrieb Russell King (Oracle):
> On Mon, Jan 22, 2024 at 10:28:41PM +0100, Dimitri Fedrau wrote:
>
> int tmp;
>
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > + MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;
>
> tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK, ret);
> *val = (tmp - 75) * 1000;
>
> > + return 0;
> > + case hwmon_temp_max:
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > + MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
> > + MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
> > + 1000;
>
> tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
> ret);
> *val = (tmp - 75) * 1000;
>
> > + return 0;
> > + case hwmon_temp_alarm:
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > + MDIO_MMD_PCS_MV_TEMP_SENSOR1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
> > + return 0;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int mv88q2xxx_hwmon_write(struct device *dev,
> > + enum hwmon_sensor_types type, u32 attr,
> > + int channel, long val)
> > +{
> > + struct phy_device *phydev = dev_get_drvdata(dev);
> > +
> > + switch (attr) {
> > + case hwmon_temp_max:
> > + if (val < -75000 || val > 180000)
> > + return -EINVAL;
> > +
> > + val = ((val / 1000) + 75) <<
> > + MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;
>
> val = (val / 1000) + 75;
> val = FIELD_PREP(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
> val);
>
> ... and therefore no need for the _SHIFT constants.
>
Will fix it.
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
© 2016 - 2025 Red Hat, Inc.