LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
@ 2021-09-02 19:09 hnagalla
  2021-09-02 23:23 ` Andrew Lunn
  0 siblings, 1 reply; 8+ messages in thread
From: hnagalla @ 2021-09-02 19:09 UTC (permalink / raw)
  To: davem, kuba; +Cc: netdev, linux-kernel, geet.modi, vikram.sharma, hnagalla

From: Hari Nagalla <hnagalla@ti.com>

Disable the over voltage interrupt at initialization to meet typical
application requirement.

Signed-off-by: Hari Nagalla <hnagalla@ti.com>
Signed-off-by: Geet Modi <geet.modi@ti.com>
Signed-off-by: Vikram Sharma <vikram.sharma@ti.com>
---
 drivers/net/phy/dp83tc811.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/phy/dp83tc811.c b/drivers/net/phy/dp83tc811.c
index 7ea32fb77190..452a39d96bd8 100644
--- a/drivers/net/phy/dp83tc811.c
+++ b/drivers/net/phy/dp83tc811.c
@@ -226,7 +226,6 @@ static int dp83811_config_intr(struct phy_device *phydev)
 				DP83811_POLARITY_INT_EN |
 				DP83811_SLEEP_MODE_INT_EN |
 				DP83811_OVERTEMP_INT_EN |
-				DP83811_OVERVOLTAGE_INT_EN |
 				DP83811_UNDERVOLTAGE_INT_EN);
 
 		err = phy_write(phydev, MII_DP83811_INT_STAT2, misr_status);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
  2021-09-02 19:09 [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization hnagalla
@ 2021-09-02 23:23 ` Andrew Lunn
  2021-09-06 20:51   ` [EXTERNAL] " Modi, Geet
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2021-09-02 23:23 UTC (permalink / raw)
  To: hnagalla; +Cc: davem, kuba, netdev, linux-kernel, geet.modi, vikram.sharma

On Thu, Sep 02, 2021 at 02:09:44PM -0500, hnagalla@ti.com wrote:
> From: Hari Nagalla <hnagalla@ti.com>
> 
> Disable the over voltage interrupt at initialization to meet typical
> application requirement.

Are you saying it is typical to supply too high a voltage?

    Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
  2021-09-02 23:23 ` Andrew Lunn
@ 2021-09-06 20:51   ` Modi, Geet
  2021-09-07 14:02     ` Andrew Lunn
  0 siblings, 1 reply; 8+ messages in thread
From: Modi, Geet @ 2021-09-06 20:51 UTC (permalink / raw)
  To: Andrew Lunn, Nagalla, Hari
  Cc: davem, kuba, netdev, linux-kernel, Sharma, Vikram

Hi Andrew,

This feature is not used by our mainstream customers as they have additional mechanism to monitor the supply at System level. 

Hence want to keep it disable by default.

Regards,
Geet


On 9/2/21, 4:23 PM, "Andrew Lunn" <andrew@lunn.ch> wrote:

    On Thu, Sep 02, 2021 at 02:09:44PM -0500, hnagalla@ti.com wrote:
    > From: Hari Nagalla <hnagalla@ti.com>
    > 
    > Disable the over voltage interrupt at initialization to meet typical
    > application requirement.

    Are you saying it is typical to supply too high a voltage?

        Andrew


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
  2021-09-06 20:51   ` [EXTERNAL] " Modi, Geet
@ 2021-09-07 14:02     ` Andrew Lunn
       [not found]       ` <E61A9519-DBA6-4931-A2A0-78856819C362@ti.com>
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2021-09-07 14:02 UTC (permalink / raw)
  To: Modi, Geet
  Cc: Nagalla, Hari, davem, kuba, netdev, linux-kernel, Sharma, Vikram

On Mon, Sep 06, 2021 at 08:51:53PM +0000, Modi, Geet wrote:
> Hi Andrew,
> 
> This feature is not used by our mainstream customers as they have additional mechanism to monitor the supply at System level. 
> 
> Hence want to keep it disable by default.

So we are slowly getting closer to a usable commit message. One that
actually makes sense.

Now, this is not really your driver, it is the Linux kernel
driver. Could somebody be using this feature? Not one of your
mainstream customer, but a Linux kernel user?

Lets look at the driver, what interrupts actually get enabled?

                misr_status |= (DP83811_RX_ERR_HF_INT_EN |
                                DP83811_MS_TRAINING_INT_EN |
                                DP83811_ANEG_COMPLETE_INT_EN |
                                DP83811_ESD_EVENT_INT_EN |
                                DP83811_WOL_INT_EN |
                                DP83811_LINK_STAT_INT_EN |
                                DP83811_ENERGY_DET_INT_EN |
                                DP83811_LINK_QUAL_INT_EN);

		misr_status |= (DP83811_JABBER_DET_INT_EN |
                                DP83811_POLARITY_INT_EN |
                                DP83811_SLEEP_MODE_INT_EN |
                                DP83811_OVERTEMP_INT_EN |
                                DP83811_OVERVOLTAGE_INT_EN |
                                DP83811_UNDERVOLTAGE_INT_EN);

Some of these i have no idea what they even do. Why would i be
interested in RX_ERR_HF_INT_EN or MS_TRAINING_INT_EN?
ESD_EVENT_INT_EN? Do we need to wake up the phy driver and update the
status because these interrupts have fired?

ANEG_COMPLETE_INT_EN, ANEG_COMPLETE_INT_EN, LINK_STAT_INT_EN make
sense.

LINK_QUAL_INT_EN and POLARITY_INT_EN could in theory make sense, but
the driver is missing the code to report quality and MDIX/MDX. 

If the driver ever gets HWMON support, OVERTEMP_INT_EN,
OVERVOLTAGE_INT_EN and UNDERVOLTAGE_INT_EN could be interesting. But
there is no HWMON support.

So it looks like there are lots of interrupts which could be removed
because nothing happens when they fire. But then i wonder, why did you
pick just one? Does it cause some sort of problem for one of your
mainstream customers? What sort of problem? Does it affect all other
Linux users, not just your customer?

So please expand your commit message to try to answer some of these
questions.

Thanks
	Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
       [not found]       ` <E61A9519-DBA6-4931-A2A0-78856819C362@ti.com>
@ 2021-09-09 20:37         ` Andrew Lunn
  2021-09-10  0:41           ` [EXTERNAL] " Modi, Geet
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2021-09-09 20:37 UTC (permalink / raw)
  To: Modi, Geet
  Cc: Nagalla, Hari, davem, kuba, netdev, linux-kernel, Sharma, Vikram

> I am planning to have following commit msg;
> 
>  
> 
> “This feature is not used by our mainstream customers as they have additional

As i said, this is not your driver, for you customers. It is the Linux
kernel driver. Please drop all references to your customers. If you
need to address anybody, it should be the Linux community as a whole,
or maybe the users of this driver.

> mechanism to monitor the supply at System level as accuracy requirements are
> different for each application.  The device is designed with inbuilt monitor
> with interrupt disabled by default and let user choose if they want to exercise
> the monitor. However, the driver had this interrupt enabled, the request here
> is disable it by default in driver however not change in datasheet.  Let user
> of the driver review the accuracy offered by monitor and if meets the
> expectation, they can always enable it.”
 
I would much more prefer something like...

The over voltage interrupt is enabled, but if it every occurs, there is
no code to make use of it. So remove the pointless enabling of it. It
can re-enabled when HWMON support is added. For the same reason,
enabling of the interrupts DP83811_RX_ERR_HF_INT_EN,
DP83811_MS_TRAINING_INT_EN, DP83811_ESD_EVENT_INT_EN,
DP83811_ENERGY_DET_INT_EN, DP83811_LINK_QUAL_INT_EN,
DP83811_JABBER_DET_INT_EN, DP83811_POLARITY_INT_EN,
DP83811_SLEEP_MODE_INT_EN, DP83811_OVERTEMP_INT_EN,
DP83811_UNDERVOLTAGE_INT_EN is also removed, since there is no code
which acts on these interrupts.

And update the patch to fit.

      Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
  2021-09-09 20:37         ` [EXTERNAL] " Andrew Lunn
@ 2021-09-10  0:41           ` Modi, Geet
  2021-09-10  1:30             ` Andrew Lunn
  0 siblings, 1 reply; 8+ messages in thread
From: Modi, Geet @ 2021-09-10  0:41 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Nagalla, Hari, davem, kuba, netdev, linux-kernel, Sharma, Vikram

Hi Andrew,

As mentioned we want to do this in phases: 
a) this patch to disable the Overvoltage driver interrupt
b) After carefully considering other interrupts, plan a  follow-on patch to take care of other interrupts.

Patch comment will be inline with your suggestion

"The over voltage interrupt is enabled, but if it ever occurs, there is no code to make use of it. So removing it. It
    can re-enabled when HWMON support is added "

Regarfds,
Geet


On 9/9/21, 1:37 PM, "Andrew Lunn" <andrew@lunn.ch> wrote:

    > I am planning to have following commit msg;
    > 
    >  
    > 
    > “This feature is not used by our mainstream customers as they have additional

    As i said, this is not your driver, for you customers. It is the Linux
    kernel driver. Please drop all references to your customers. If you
    need to address anybody, it should be the Linux community as a whole,
    or maybe the users of this driver.

    > mechanism to monitor the supply at System level as accuracy requirements are
    > different for each application.  The device is designed with inbuilt monitor
    > with interrupt disabled by default and let user choose if they want to exercise
    > the monitor. However, the driver had this interrupt enabled, the request here
    > is disable it by default in driver however not change in datasheet.  Let user
    > of the driver review the accuracy offered by monitor and if meets the
    > expectation, they can always enable it.”

    I would much more prefer something like...

    The over voltage interrupt is enabled, but if it every occurs, there is
    no code to make use of it. So remove the pointless enabling of it. It
    can re-enabled when HWMON support is added. For the same reason,
    enabling of the interrupts DP83811_RX_ERR_HF_INT_EN,
    DP83811_MS_TRAINING_INT_EN, DP83811_ESD_EVENT_INT_EN,
    DP83811_ENERGY_DET_INT_EN, DP83811_LINK_QUAL_INT_EN,
    DP83811_JABBER_DET_INT_EN, DP83811_POLARITY_INT_EN,
    DP83811_SLEEP_MODE_INT_EN, DP83811_OVERTEMP_INT_EN,
    DP83811_UNDERVOLTAGE_INT_EN is also removed, since there is no code
    which acts on these interrupts.

    And update the patch to fit.

          Andrew


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
  2021-09-10  0:41           ` [EXTERNAL] " Modi, Geet
@ 2021-09-10  1:30             ` Andrew Lunn
  2021-09-10  1:42               ` [EXTERNAL] " Modi, Geet
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2021-09-10  1:30 UTC (permalink / raw)
  To: Modi, Geet
  Cc: Nagalla, Hari, davem, kuba, netdev, linux-kernel, Sharma, Vikram,
	dmurphy

On Fri, Sep 10, 2021 at 12:41:53AM +0000, Modi, Geet wrote:
> Hi Andrew,
> 
> As mentioned we want to do this in phases: 
> a) this patch to disable the Overvoltage driver interrupt
> b) After carefully considering other interrupts, plan a  follow-on patch to take care of other interrupts.

I still don't get it. Why just Over volt now and not the rest, which
are equally useless? It makes me think there is something seriously
wrong with over voltage, which you are not telling us about. Maybe an
interrupt storm? If there is something broken here, this patch needs
to be back ported to stable.

   Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
  2021-09-10  1:30             ` Andrew Lunn
@ 2021-09-10  1:42               ` Modi, Geet
  0 siblings, 0 replies; 8+ messages in thread
From: Modi, Geet @ 2021-09-10  1:42 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Nagalla, Hari, davem, kuba, netdev, linux-kernel, Sharma, Vikram,
	dmurphy

Hi Andrew,

Please be assure the monitors are part of the PHY and well captured in device datasheet.   The only reason to go selectively is as we have not carefully reviewed the other  interrupts usage by application, hence don't want to make the change in haste.

Regards,
Geet




On 9/9/21, 6:31 PM, "Andrew Lunn" <andrew@lunn.ch> wrote:

    On Fri, Sep 10, 2021 at 12:41:53AM +0000, Modi, Geet wrote:
    > Hi Andrew,
    > 
    > As mentioned we want to do this in phases: 
    > a) this patch to disable the Overvoltage driver interrupt
    > b) After carefully considering other interrupts, plan a  follow-on patch to take care of other interrupts.

    I still don't get it. Why just Over volt now and not the rest, which
    are equally useless? It makes me think there is something seriously
    wrong with over voltage, which you are not telling us about. Maybe an
    interrupt storm? If there is something broken here, this patch needs
    to be back ported to stable.

       Andrew


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-10  1:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 19:09 [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization hnagalla
2021-09-02 23:23 ` Andrew Lunn
2021-09-06 20:51   ` [EXTERNAL] " Modi, Geet
2021-09-07 14:02     ` Andrew Lunn
     [not found]       ` <E61A9519-DBA6-4931-A2A0-78856819C362@ti.com>
2021-09-09 20:37         ` [EXTERNAL] " Andrew Lunn
2021-09-10  0:41           ` [EXTERNAL] " Modi, Geet
2021-09-10  1:30             ` Andrew Lunn
2021-09-10  1:42               ` [EXTERNAL] " Modi, Geet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).