* [PATCH net-next v2 1/3] net: phy: enable interrupts when PHY is attached already
2019-05-30 13:08 [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's Heiner Kallweit
@ 2019-05-30 13:09 ` Heiner Kallweit
2019-05-30 19:52 ` Andrew Lunn
2019-05-30 13:10 ` [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver Heiner Kallweit
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Heiner Kallweit @ 2019-05-30 13:09 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn, Florian Fainelli, David Miller
Cc: netdev
This patch is a step towards allowing PHY drivers to handle more
interrupt sources than just link change. E.g. several PHY's have
built-in temperature monitoring and can raise an interrupt if a
temperature threshold is exceeded. We may be interested in such
interrupts also if the phylib state machine isn't started.
Therefore move enabling interrupts to phy_request_interrupt().
v2:
- patch added to series
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy.c | 36 ++++++++++++++++++++++--------------
drivers/net/phy/phy_device.c | 2 +-
include/linux/phy.h | 1 +
3 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e88854292..d90d9863e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -799,10 +799,10 @@ static int phy_enable_interrupts(struct phy_device *phydev)
}
/**
- * phy_request_interrupt - request interrupt for a PHY device
+ * phy_request_interrupt - request and enable interrupt for a PHY device
* @phydev: target phy_device struct
*
- * Description: Request the interrupt for the given PHY.
+ * Description: Request and enable the interrupt for the given PHY.
* If this fails, then we set irq to PHY_POLL.
* This should only be called with a valid IRQ number.
*/
@@ -817,10 +817,30 @@ void phy_request_interrupt(struct phy_device *phydev)
phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n",
err, phydev->irq);
phydev->irq = PHY_POLL;
+ } else {
+ if (phy_enable_interrupts(phydev)) {
+ phydev_warn(phydev, "Can't enable interrupt, falling back to polling\n");
+ phy_free_interrupt(phydev);
+ phydev->irq = PHY_POLL;
+ }
}
}
EXPORT_SYMBOL(phy_request_interrupt);
+/**
+ * phy_free_interrupt - disable and free interrupt for a PHY device
+ * @phydev: target phy_device struct
+ *
+ * Description: Disable and free the interrupt for the given PHY.
+ * This should only be called with a valid IRQ number.
+ */
+void phy_free_interrupt(struct phy_device *phydev)
+{
+ phy_disable_interrupts(phydev);
+ free_irq(phydev->irq, phydev);
+}
+EXPORT_SYMBOL(phy_free_interrupt);
+
/**
* phy_stop - Bring down the PHY link, and stop checking the status
* @phydev: target phy_device struct
@@ -835,9 +855,6 @@ void phy_stop(struct phy_device *phydev)
mutex_lock(&phydev->lock);
- if (phy_interrupt_is_valid(phydev))
- phy_disable_interrupts(phydev);
-
phydev->state = PHY_HALTED;
mutex_unlock(&phydev->lock);
@@ -864,8 +881,6 @@ EXPORT_SYMBOL(phy_stop);
*/
void phy_start(struct phy_device *phydev)
{
- int err;
-
mutex_lock(&phydev->lock);
if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
@@ -877,13 +892,6 @@ void phy_start(struct phy_device *phydev)
/* if phy was suspended, bring the physical link up again */
__phy_resume(phydev);
- /* make sure interrupts are enabled for the PHY */
- if (phy_interrupt_is_valid(phydev)) {
- err = phy_enable_interrupts(phydev);
- if (err < 0)
- goto out;
- }
-
phydev->state = PHY_UP;
phy_start_machine(phydev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5d288da9a..d71b3ed52 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1013,7 +1013,7 @@ void phy_disconnect(struct phy_device *phydev)
phy_stop(phydev);
if (phy_interrupt_is_valid(phydev))
- free_irq(phydev->irq, phydev);
+ phy_free_interrupt(phydev);
phydev->adjust_link = NULL;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7180b1d1e..72e1196f9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1147,6 +1147,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
const struct ethtool_link_ksettings *cmd);
int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd);
void phy_request_interrupt(struct phy_device *phydev);
+void phy_free_interrupt(struct phy_device *phydev);
void phy_print_status(struct phy_device *phydev);
int phy_set_max_speed(struct phy_device *phydev, u32 max_speed);
void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 1/3] net: phy: enable interrupts when PHY is attached already
2019-05-30 13:09 ` [PATCH net-next v2 1/3] net: phy: enable interrupts when PHY is attached already Heiner Kallweit
@ 2019-05-30 19:52 ` Andrew Lunn
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2019-05-30 19:52 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Russell King - ARM Linux, Florian Fainelli, David Miller, netdev
On Thu, May 30, 2019 at 03:09:15PM +0200, Heiner Kallweit wrote:
> This patch is a step towards allowing PHY drivers to handle more
> interrupt sources than just link change. E.g. several PHY's have
> built-in temperature monitoring and can raise an interrupt if a
> temperature threshold is exceeded. We may be interested in such
> interrupts also if the phylib state machine isn't started.
> Therefore move enabling interrupts to phy_request_interrupt().
>
> v2:
> - patch added to series
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver
2019-05-30 13:08 [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's Heiner Kallweit
2019-05-30 13:09 ` [PATCH net-next v2 1/3] net: phy: enable interrupts when PHY is attached already Heiner Kallweit
@ 2019-05-30 13:10 ` Heiner Kallweit
2019-11-19 10:33 ` Michael Walle
2019-05-30 13:11 ` [PATCH net-next v2 3/3] net: phy: export phy_queue_state_machine Heiner Kallweit
2019-05-30 22:02 ` [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's David Miller
3 siblings, 1 reply; 8+ messages in thread
From: Heiner Kallweit @ 2019-05-30 13:10 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn, Florian Fainelli, David Miller
Cc: netdev
The phylib interrupt handler handles link change events only currently.
However PHY drivers may want to use other interrupt sources too,
e.g. to report temperature monitoring events. Therefore add a callback
to struct phy_driver allowing PHY drivers to implement a custom
interrupt handler.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy.c | 9 +++++++--
include/linux/phy.h | 3 +++
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d90d9863e..068f0a126 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -772,8 +772,13 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
return IRQ_NONE;
- /* reschedule state queue work to run as soon as possible */
- phy_trigger_machine(phydev);
+ if (phydev->drv->handle_interrupt) {
+ if (phydev->drv->handle_interrupt(phydev))
+ goto phy_err;
+ } else {
+ /* reschedule state queue work to run as soon as possible */
+ phy_trigger_machine(phydev);
+ }
if (phy_clear_interrupt(phydev))
goto phy_err;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 72e1196f9..16cd33915 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -537,6 +537,9 @@ struct phy_driver {
*/
int (*did_interrupt)(struct phy_device *phydev);
+ /* Override default interrupt handling */
+ int (*handle_interrupt)(struct phy_device *phydev);
+
/* Clears up any memory if needed */
void (*remove)(struct phy_device *phydev);
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver
2019-05-30 13:10 ` [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver Heiner Kallweit
@ 2019-11-19 10:33 ` Michael Walle
2019-11-19 10:50 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 8+ messages in thread
From: Michael Walle @ 2019-11-19 10:33 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, davem, f.fainelli, linux, netdev
Hi,
this is an old thread and I know its already applied. But I'd like to
hear your opinion on the following problem below.
> The phylib interrupt handler handles link change events only currently.
> However PHY drivers may want to use other interrupt sources too,
> e.g. to report temperature monitoring events. Therefore add a callback
> to struct phy_driver allowing PHY drivers to implement a custom
> interrupt handler.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/net/phy/phy.c | 9 +++++++--
> include/linux/phy.h | 3 +++
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d90d9863e..068f0a126 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -772,8 +772,13 @@ static irqreturn_t phy_interrupt(int irq, void
> *phy_dat)
> if (phydev->drv->did_interrupt &&
> !phydev->drv->did_interrupt(phydev))
> return IRQ_NONE;
>
> - /* reschedule state queue work to run as soon as possible */
> - phy_trigger_machine(phydev);
> + if (phydev->drv->handle_interrupt) {
> + if (phydev->drv->handle_interrupt(phydev))
> + goto phy_err;
There are PHYs which clears the interrupt already by reading the
interrupt status register. To do something useful in handle_interrupt()
I have to read the interrupt status register, thus clearing the pending
interrupts.
> + } else {
> + /* reschedule state queue work to run as soon as possible */
> + phy_trigger_machine(phydev);
> + }
>
> if (phy_clear_interrupt(phydev))
> goto phy_err;
But here the interrupts are cleared again, which means we might loose
interrupt causes in between.
I could think of two different fixes:
(1) handle_interrupt() has to take care to clear the interrupts and
skip the phy_clear_interrupt() above.
(2) handle_interrupt() might return a special return code which skips
the phy_clear_interrupt
TBH, I'd prefer (1) but I don't know if it is allowed to change
semantics afterwards. (Also, I've found no driver where
handle_interrupt() is actually used for now?)
-michael
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver
2019-11-19 10:33 ` Michael Walle
@ 2019-11-19 10:50 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 8+ messages in thread
From: Russell King - ARM Linux admin @ 2019-11-19 10:50 UTC (permalink / raw)
To: Michael Walle; +Cc: hkallweit1, andrew, davem, f.fainelli, netdev
On Tue, Nov 19, 2019 at 11:33:47AM +0100, Michael Walle wrote:
>
> Hi,
>
> this is an old thread and I know its already applied. But I'd like to hear
> your opinion on the following problem below.
>
> > The phylib interrupt handler handles link change events only currently.
> > However PHY drivers may want to use other interrupt sources too,
> > e.g. to report temperature monitoring events. Therefore add a callback
> > to struct phy_driver allowing PHY drivers to implement a custom
> > interrupt handler.
> >
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > drivers/net/phy/phy.c | 9 +++++++--
> > include/linux/phy.h | 3 +++
> > 2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index d90d9863e..068f0a126 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -772,8 +772,13 @@ static irqreturn_t phy_interrupt(int irq, void
> > *phy_dat)
> > if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
> > return IRQ_NONE;
> >
> > - /* reschedule state queue work to run as soon as possible */
> > - phy_trigger_machine(phydev);
> > + if (phydev->drv->handle_interrupt) {
> > + if (phydev->drv->handle_interrupt(phydev))
> > + goto phy_err;
>
> There are PHYs which clears the interrupt already by reading the interrupt
> status register. To do something useful in handle_interrupt() I have to read
> the interrupt status register, thus clearing the pending interrupts.
>
>
> > + } else {
> > + /* reschedule state queue work to run as soon as possible */
> > + phy_trigger_machine(phydev);
> > + }
> >
> > if (phy_clear_interrupt(phydev))
> > goto phy_err;
>
> But here the interrupts are cleared again, which means we might loose
> interrupt causes in between.
>
> I could think of two different fixes:
> (1) handle_interrupt() has to take care to clear the interrupts and skip
> the phy_clear_interrupt() above.
> (2) handle_interrupt() might return a special return code which skips the
> phy_clear_interrupt
>
> TBH, I'd prefer (1) but I don't know if it is allowed to change semantics
> afterwards. (Also, I've found no driver where handle_interrupt() is actually
> used for now?)
I made the argument at the time that phylib should stop being a middle-
layer, but instead let PHY drivers take care of interrupt handling
themselves, just like we do elsewhere in the kernel. I think your
case just shows that trying to keep the interrupt handling structured
inside phylib and trying to make all PHYs fit is just going to be
painful.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v2 3/3] net: phy: export phy_queue_state_machine
2019-05-30 13:08 [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's Heiner Kallweit
2019-05-30 13:09 ` [PATCH net-next v2 1/3] net: phy: enable interrupts when PHY is attached already Heiner Kallweit
2019-05-30 13:10 ` [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver Heiner Kallweit
@ 2019-05-30 13:11 ` Heiner Kallweit
2019-05-30 22:02 ` [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's David Miller
3 siblings, 0 replies; 8+ messages in thread
From: Heiner Kallweit @ 2019-05-30 13:11 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn, Florian Fainelli, David Miller
Cc: netdev
We face the issue that link change interrupt and link status may be
reported by different PHY layers. As a result the link change
interrupt may occur before the link status changes.
Export phy_queue_state_machine to allow PHY drivers to specify a
delay between link status change interrupt and link status check.
v2:
- change jiffies parameter type to unsigned long
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy.c | 8 +++++---
include/linux/phy.h | 2 +-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 068f0a126..74dfd424c 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -29,6 +29,8 @@
#include <linux/uaccess.h>
#include <linux/atomic.h>
+#define PHY_STATE_TIME HZ
+
#define PHY_STATE_STR(_state) \
case PHY_##_state: \
return __stringify(_state); \
@@ -478,12 +480,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
}
EXPORT_SYMBOL(phy_mii_ioctl);
-static void phy_queue_state_machine(struct phy_device *phydev,
- unsigned int secs)
+void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies)
{
mod_delayed_work(system_power_efficient_wq, &phydev->state_queue,
- secs * HZ);
+ jiffies);
}
+EXPORT_SYMBOL(phy_queue_state_machine);
static void phy_trigger_machine(struct phy_device *phydev)
{
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 16cd33915..dc4b51060 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -188,7 +188,6 @@ static inline const char *phy_modes(phy_interface_t interface)
#define PHY_INIT_TIMEOUT 100000
-#define PHY_STATE_TIME 1
#define PHY_FORCE_TIMEOUT 10
#define PHY_MAX_ADDR 32
@@ -1140,6 +1139,7 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
int phy_drivers_register(struct phy_driver *new_driver, int n,
struct module *owner);
void phy_state_machine(struct work_struct *work);
+void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies);
void phy_mac_interrupt(struct phy_device *phydev);
void phy_start_machine(struct phy_device *phydev);
void phy_stop_machine(struct phy_device *phydev);
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's
2019-05-30 13:08 [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's Heiner Kallweit
` (2 preceding siblings ...)
2019-05-30 13:11 ` [PATCH net-next v2 3/3] net: phy: export phy_queue_state_machine Heiner Kallweit
@ 2019-05-30 22:02 ` David Miller
3 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2019-05-30 22:02 UTC (permalink / raw)
To: hkallweit1; +Cc: linux, andrew, f.fainelli, netdev
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 30 May 2019 15:08:09 +0200
> This series tries to address few problematic aspects raised by
> Russell. Concrete example is the Marvell 88x3310, the changes
> should be helpful for other complex C45 PHY's too.
>
> v2:
> - added patch enabling interrupts also if phylib state machine
> isn't started
> - removed patch dealing with the double link status read
> This one needs little bit more thinking and will go separately.
Series applied.
^ permalink raw reply [flat|nested] 8+ messages in thread