> +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
> 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
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
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 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