[PATCH] net: phy: Support for non-HW interrupt devices

[PATCH] net: phy: Support for non-HW interrupt devices

[Woojung.Huh]

This patch introduces PHY_PSEUDO_INTERRUPT and routines to
handle pseudo (not-real-hardware) interrupt such as USB interrupt pipe for
USB-to-Ethernet device.

Unless having real hardware interrupt handler registered by request_irq(),
phy_state_machine() can't avoid calling phy_read_status()
to monitor link changes. This especially prevents USB-to-Ethernet device
from going to auto suspend to save power.

Signed-off-by: Woojung Huh <woojung.huh@microchip.com>
---
 drivers/net/phy/phy.c | 31 +++++++++++++++++++++++++++++--
 include/linux/phy.h   |  6 ++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 47cd306..8f678e9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -588,6 +588,25 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 }
 
 /**
+ * phy_pseudo_interrupt - PHY pseudo interrupt handler
+ * @phydev: target phy_device struct
+ *
+ * Description: This is for phy pseudo interrupt such as
+ * USB interrupt pipe for USB-to-Ethernet devices.
+ * When a PHY pseudo interrupt occurs, i.e, USB interrupt pipe completion,
+ * the handler schedules a work task to clear the interrupt.
+ */
+void phy_pseudo_interrupt(struct phy_device *phydev)
+{
+	if (phydev->state != PHY_HALTED) {
+		atomic_inc(&phydev->irq_disable);
+
+		queue_work(system_power_efficient_wq, &phydev->phy_queue);
+	}
+}
+EXPORT_SYMBOL(phy_pseudo_interrupt);
+
+/**
  * phy_enable_interrupts - Enable the interrupts from the PHY side
  * @phydev: target phy_device struct
  */
@@ -640,12 +659,20 @@ phy_err:
 int phy_start_interrupts(struct phy_device *phydev)
 {
 	atomic_set(&phydev->irq_disable, 0);
-	if (request_irq(phydev->irq, phy_interrupt, 0, "phy_interrupt",
-			phydev) < 0) {
+
+	/* phydev->irq is bigger than zero when real H/W interrupt.
+	 * This avoids calling request_irq when pseudo interrupt such as
+	 * USB interrupt pipe for USB-to-Ethernet device.
+	 */
+	if ((phydev->irq > 0) &&
+	    (request_irq(phydev->irq, phy_interrupt, 0, "phy_interrupt",
+			 phydev) < 0)) {
 		pr_warn("%s: Can't get IRQ %d (PHY)\n",
 			phydev->bus->name, phydev->irq);
 		phydev->irq = PHY_POLL;
 		return 0;
+	} else if (phydev->irq == PHY_PSEUDO_INTERRUPT) {
+		phy_pseudo_interrupt(phydev);
 	}
 
 	return phy_enable_interrupts(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 05fde31..a68e690 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -55,6 +55,12 @@
 #define PHY_POLL		-1
 #define PHY_IGNORE_INTERRUPT	-2
 
+/*
+ * Set to PHY_PSEUDO_INTERRUPT when the attached driver uses
+ * not real hardware interrupt such as USB interrupt pipe.
+ */
+#define PHY_PSEUDO_INTERRUPT	-100
+
 #define PHY_HAS_INTERRUPT	0x00000001
 #define PHY_HAS_MAGICANEG	0x00000002
 #define PHY_IS_INTERNAL		0x00000004
-- 
2.1.4

Re: [PATCH] net: phy: Support for non-HW interrupt devices

[Andrew Lunn]

On Thu, Jan 07, 2016 at 11:14:19PM +0000, Woojung.Huh@microchip.com wrote:
> This patch introduces PHY_PSEUDO_INTERRUPT and routines to
> handle pseudo (not-real-hardware) interrupt such as USB interrupt pipe for
> USB-to-Ethernet device.
> 
> Unless having real hardware interrupt handler registered by request_irq(),
> phy_state_machine() can't avoid calling phy_read_status()
> to monitor link changes. This especially prevents USB-to-Ethernet device
> from going to auto suspend to save power.

Hi Woojung

Do you have an example PHY driver making use of this. At the moment i
don't see how it works.

There is possibly a cleaner way to do this. I've some unpublished work
where i added interrupt support for the Marvell switches. These
switches can have embedded PHYs, and the interrupts from these PHYs go
into the switchers interrupt controller. From that one line comes out
and is connected to a GPIO line. To support this, i've implemented a
Linux chained interrupt driver within the switch driver. PHY
interrupts then become normal Linux interrupts which the PHY library
can make use of.

So, maybe your USB driver should implemented a Linux interrupt
controller. USB interrupt pipe transactions result in the interrupt
controller triggering the normal Linux interrupt mechanism, and so no
need to change PHYLIB.

In fact, USB interrupt transaction to Linux interrupt sounds generic
enough that it might of been implemented already somewhere.

       Andrew

> 
> Signed-off-by: Woojung Huh <woojung.huh@microchip.com>
> ---
>  drivers/net/phy/phy.c | 31 +++++++++++++++++++++++++++++--
>  include/linux/phy.h   |  6 ++++++
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 47cd306..8f678e9 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -588,6 +588,25 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>  }
>  
>  /**
> + * phy_pseudo_interrupt - PHY pseudo interrupt handler
> + * @phydev: target phy_device struct
> + *
> + * Description: This is for phy pseudo interrupt such as
> + * USB interrupt pipe for USB-to-Ethernet devices.
> + * When a PHY pseudo interrupt occurs, i.e, USB interrupt pipe completion,
> + * the handler schedules a work task to clear the interrupt.
> + */
> +void phy_pseudo_interrupt(struct phy_device *phydev)
> +{
> +	if (phydev->state != PHY_HALTED) {
> +		atomic_inc(&phydev->irq_disable);
> +
> +		queue_work(system_power_efficient_wq, &phydev->phy_queue);
> +	}
> +}
> +EXPORT_SYMBOL(phy_pseudo_interrupt);
> +
> +/**
>   * phy_enable_interrupts - Enable the interrupts from the PHY side
>   * @phydev: target phy_device struct
>   */
> @@ -640,12 +659,20 @@ phy_err:
>  int phy_start_interrupts(struct phy_device *phydev)
>  {
>  	atomic_set(&phydev->irq_disable, 0);
> -	if (request_irq(phydev->irq, phy_interrupt, 0, "phy_interrupt",
> -			phydev) < 0) {
> +
> +	/* phydev->irq is bigger than zero when real H/W interrupt.
> +	 * This avoids calling request_irq when pseudo interrupt such as
> +	 * USB interrupt pipe for USB-to-Ethernet device.
> +	 */
> +	if ((phydev->irq > 0) &&
> +	    (request_irq(phydev->irq, phy_interrupt, 0, "phy_interrupt",
> +			 phydev) < 0)) {
>  		pr_warn("%s: Can't get IRQ %d (PHY)\n",
>  			phydev->bus->name, phydev->irq);
>  		phydev->irq = PHY_POLL;
>  		return 0;
> +	} else if (phydev->irq == PHY_PSEUDO_INTERRUPT) {
> +		phy_pseudo_interrupt(phydev);
>  	}
>  
>  	return phy_enable_interrupts(phydev);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 05fde31..a68e690 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -55,6 +55,12 @@
>  #define PHY_POLL		-1
>  #define PHY_IGNORE_INTERRUPT	-2
>  
> +/*
> + * Set to PHY_PSEUDO_INTERRUPT when the attached driver uses
> + * not real hardware interrupt such as USB interrupt pipe.
> + */
> +#define PHY_PSEUDO_INTERRUPT	-100
> +
>  #define PHY_HAS_INTERRUPT	0x00000001
>  #define PHY_HAS_MAGICANEG	0x00000002
>  #define PHY_IS_INTERNAL		0x00000004
> -- 
> 2.1.4

Re: [PATCH] net: phy: Support for non-HW interrupt devices

[Florian Fainelli]

On 07/01/16 16:42, Andrew Lunn wrote:
> On Thu, Jan 07, 2016 at 11:14:19PM +0000, Woojung.Huh@microchip.com wrote:
>> This patch introduces PHY_PSEUDO_INTERRUPT and routines to
>> handle pseudo (not-real-hardware) interrupt such as USB interrupt pipe for
>> USB-to-Ethernet device.
>>
>> Unless having real hardware interrupt handler registered by request_irq(),
>> phy_state_machine() can't avoid calling phy_read_status()
>> to monitor link changes. This especially prevents USB-to-Ethernet device
>> from going to auto suspend to save power.

Well, this surely is something that can be fixed.

> 
> Hi Woojung
> 
> Do you have an example PHY driver making use of this. At the moment i
> don't see how it works.

I would assume that the changes would be in the Ethernet portion of the
driver and maybe also in the PHY driver to ack/configure interrupts. As
Andrew indicated, you probably want to post the full patch series to
show how this will be used.

> 
> There is possibly a cleaner way to do this. I've some unpublished work
> where i added interrupt support for the Marvell switches. These
> switches can have embedded PHYs, and the interrupts from these PHYs go
> into the switchers interrupt controller. From that one line comes out
> and is connected to a GPIO line. To support this, i've implemented a
> Linux chained interrupt driver within the switch driver. PHY
> interrupts then become normal Linux interrupts which the PHY library
> can make use of.

That seems like an elegant way to solve the problem, without adding code
in the PHY library that does not use the interrupt API, but it also
seems to me like there are some other potential issues associated with
doing that:

- if a Switch/Ethernet controller has several interrupt bits
corresponding to a built-in PHY (link UP, DOWN, Energy detect), PHYLIB
still requests only one interrupt, and expects it to be able to read the
interrupt cause and do something with it, if you interface an interrupt
controller in the middle, that might go against being able to do that?

- does that scheme work well with Device Tree if you dynamically
register an interrupt domain? of_mdiobus_register_phy() will try to
parse a standard "interrupts" property for the PHY device, and fill in
the mii_bus->irq for that PHY address, sure you could still override
that later with an interrupt domain, but that sounds a little error probe

- this adds a bit of complexity to an Ethernet/Switch driver to get PHY
devices to be probed successfully, since you now need to register an
interrupt controller driver/domain, and do that before your MDIO bus
driver is probed and your Ethernet MAC issues a phy_connect(), not
impossible to get right, just a bit more complex

Do not get me wrong, I do believe using the existing interrupt
abstraction is the right answer, I am just wondering how we should be
dealing with that at the PHY library level, in the case where there are
different interrupt sources available (the most common being link
UP/DOWN events) and how nice would that play with different drivers out
there.

> 
> So, maybe your USB driver should implemented a Linux interrupt
> controller. USB interrupt pipe transactions result in the interrupt
> controller triggering the normal Linux interrupt mechanism, and so no
> need to change PHYLIB.

Right, that seems like a potential option here. phy_mac_interrupt() was
intended to be used for that kind of situation where the interrupt for
the PHY is outside of the control of the PHY driver, but unfortunately I
think it still makes the PHY library poll the PHY, which should
definitively be fixed.
-- 
Florian

RE: [PATCH] net: phy: Support for non-HW interrupt devices

[Woojung.Huh]

Hi Florian & Andrew,

Thanks for your feedback and I'm glad that there are requests to expand PHYLIB.
I'm sorry not to post sample code to use this patch. I'll post another patch with
sample I'm testing.

As you may already know, USB Interrupt Pipe is not actual interrupt and
USB driver is beyond USB Host driver which deals with HW interrupt directly over
request_irq(). So even we can put some code in USB Host driver to interface with 
PHYLIB, it is more likely adding another USB stack to handle this pseudo interrupt.
I don't think it's good approach to solve this problem.

Woojung
________________________________________
From: Florian Fainelli [f.fainelli@gmail.com]
Sent: Thursday, January 07, 2016 9:08 PM
To: Andrew Lunn; Woojung Huh - C21699
Cc: netdev@vger.kernel.org; davem@davemloft.net
Subject: Re: [PATCH] net: phy: Support for non-HW interrupt devices

On 07/01/16 16:42, Andrew Lunn wrote:
> On Thu, Jan 07, 2016 at 11:14:19PM +0000, Woojung.Huh@microchip.com wrote:
>> This patch introduces PHY_PSEUDO_INTERRUPT and routines to
>> handle pseudo (not-real-hardware) interrupt such as USB interrupt pipe for
>> USB-to-Ethernet device.
>>
>> Unless having real hardware interrupt handler registered by request_irq(),
>> phy_state_machine() can't avoid calling phy_read_status()
>> to monitor link changes. This especially prevents USB-to-Ethernet device
>> from going to auto suspend to save power.

Well, this surely is something that can be fixed.

>
> Hi Woojung
>
> Do you have an example PHY driver making use of this. At the moment i
> don't see how it works.

I would assume that the changes would be in the Ethernet portion of the
driver and maybe also in the PHY driver to ack/configure interrupts. As
Andrew indicated, you probably want to post the full patch series to
show how this will be used.

>
> There is possibly a cleaner way to do this. I've some unpublished work
> where i added interrupt support for the Marvell switches. These
> switches can have embedded PHYs, and the interrupts from these PHYs go
> into the switchers interrupt controller. From that one line comes out
> and is connected to a GPIO line. To support this, i've implemented a
> Linux chained interrupt driver within the switch driver. PHY
> interrupts then become normal Linux interrupts which the PHY library
> can make use of.

That seems like an elegant way to solve the problem, without adding code
in the PHY library that does not use the interrupt API, but it also
seems to me like there are some other potential issues associated with
doing that:

- if a Switch/Ethernet controller has several interrupt bits
corresponding to a built-in PHY (link UP, DOWN, Energy detect), PHYLIB
still requests only one interrupt, and expects it to be able to read the
interrupt cause and do something with it, if you interface an interrupt
controller in the middle, that might go against being able to do that?

- does that scheme work well with Device Tree if you dynamically
register an interrupt domain? of_mdiobus_register_phy() will try to
parse a standard "interrupts" property for the PHY device, and fill in
the mii_bus->irq for that PHY address, sure you could still override
that later with an interrupt domain, but that sounds a little error probe

- this adds a bit of complexity to an Ethernet/Switch driver to get PHY
devices to be probed successfully, since you now need to register an
interrupt controller driver/domain, and do that before your MDIO bus
driver is probed and your Ethernet MAC issues a phy_connect(), not
impossible to get right, just a bit more complex

Do not get me wrong, I do believe using the existing interrupt
abstraction is the right answer, I am just wondering how we should be
dealing with that at the PHY library level, in the case where there are
different interrupt sources available (the most common being link
UP/DOWN events) and how nice would that play with different drivers out
there.

>
> So, maybe your USB driver should implemented a Linux interrupt
> controller. USB interrupt pipe transactions result in the interrupt
> controller triggering the normal Linux interrupt mechanism, and so no
> need to change PHYLIB.

Right, that seems like a potential option here. phy_mac_interrupt() was
intended to be used for that kind of situation where the interrupt for
the PHY is outside of the control of the PHY driver, but unfortunately I
think it still makes the PHY library poll the PHY, which should
definitively be fixed.

Re: [PATCH] net: phy: Support for non-HW interrupt devices

[Andrew Lunn]

> As you may already know, USB Interrupt Pipe is not actual interrupt and
> USB driver is beyond USB Host driver which deals with HW interrupt directly over
> request_irq(). So even we can put some code in USB Host driver to interface with 
> PHYLIB, it is more likely adding another USB stack to handle this pseudo interrupt.
> I don't think it's good approach to solve this problem.

You don't need any code in the USB irq handler.

I'm not too familiar with USB networking, but looking at usbnet.c, it
looks like intr_complete() gets called when there is a USB Interrupt
Pipe event. That calls into the drivers status() method.

In this status method, you need to trigger a Linux interrupt. By that,
i mean you need to call handle_nested_irq().

What i found useful was looking at how drivers/gpio/gpip-pca953x.c
works. That is an i2c GPIO expander, which supports GPIO
interrupts. When the device indicates an interrupt event, you need to
read an i2c register on the device to see which GPIO line has
triggered it, and then call handle_nestd_irq() for the corresponding
interrupt.

Humm, in fact, take a look at drivers/gpio/gpio-dln2.c It is a USB
GPIO expander, with interrupt support.

	Andrew

RE: [PATCH] net: phy: Support for non-HW interrupt devices

[Woojung.Huh]

Thanks for your information. I'll take a look.

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Friday, January 08, 2016 10:51 AM
> To: Woojung Huh - C21699
> Cc: f.fainelli@gmail.com; netdev@vger.kernel.org; davem@davemloft.net
> Subject: Re: [PATCH] net: phy: Support for non-HW interrupt devices
> 
> > As you may already know, USB Interrupt Pipe is not actual interrupt and
> > USB driver is beyond USB Host driver which deals with HW interrupt directly
> over
> > request_irq(). So even we can put some code in USB Host driver to
> interface with
> > PHYLIB, it is more likely adding another USB stack to handle this pseudo
> interrupt.
> > I don't think it's good approach to solve this problem.
> 
> You don't need any code in the USB irq handler.
> 
> I'm not too familiar with USB networking, but looking at usbnet.c, it
> looks like intr_complete() gets called when there is a USB Interrupt
> Pipe event. That calls into the drivers status() method.
> 
> In this status method, you need to trigger a Linux interrupt. By that,
> i mean you need to call handle_nested_irq().
> 
> What i found useful was looking at how drivers/gpio/gpip-pca953x.c
> works. That is an i2c GPIO expander, which supports GPIO
> interrupts. When the device indicates an interrupt event, you need to
> read an i2c register on the device to see which GPIO line has
> triggered it, and then call handle_nestd_irq() for the corresponding
> interrupt.
> 
> Humm, in fact, take a look at drivers/gpio/gpio-dln2.c It is a USB
> GPIO expander, with interrupt support.
> 
> 	Andrew

Re: [PATCH] net: phy: Support for non-HW interrupt devices

[Andrew Lunn]

> That seems like an elegant way to solve the problem, without adding code
> in the PHY library that does not use the interrupt API, but it also
> seems to me like there are some other potential issues associated with
> doing that:

Hi Florian

I don't want to hijack this thread with DSA stuff. Plus you gave me
some ideas for improvements :-)

Lets come back to this in a couple of weeks once i have the DSA
proposal patches rebased on mdio device, and i've played a bit more.

	 Andrew

Re: [PATCH] net: phy: Support for non-HW interrupt devices

[Florian Fainelli]

Hello Andrew,

Le 08/01/2016 11:24, Andrew Lunn a écrit :
>> That seems like an elegant way to solve the problem, without adding code
>> in the PHY library that does not use the interrupt API, but it also
>> seems to me like there are some other potential issues associated with
>> doing that:
> 
> Hi Florian
> 
> I don't want to hijack this thread with DSA stuff. Plus you gave me
> some ideas for improvements :-)
> 
> Lets come back to this in a couple of weeks once i have the DSA
> proposal patches rebased on mdio device, and i've played a bit more.

Sounds good, I will send some patches shortly which really fix the PHY
state machine not to poll PHYs configured with PHY_IGNORE_INTERRUPT and
fixing phy_mac_interrupt() to be callable in hard IRQ context (finally).

Maybe these patches can help Woojung get what he needs for his driver.
-- 
Florian

RE: [PATCH] net: phy: Support for non-HW interrupt devices

[Woojung.Huh]

Hi Florian,

> Sounds good, I will send some patches shortly which really fix the PHY
> state machine not to poll PHYs configured with PHY_IGNORE_INTERRUPT and
> fixing phy_mac_interrupt() to be callable in hard IRQ context (finally).
> 
> Maybe these patches can help Woojung get what he needs for his driver.

I'm happy to test new code with my driver.

Woojung

Re: [PATCH] net: phy: Support for non-HW interrupt devices

[Andrew Lunn]

> Sounds good, I will send some patches shortly which really fix the PHY
> state machine not to poll PHYs configured with PHY_IGNORE_INTERRUPT and
> fixing phy_mac_interrupt() to be callable in hard IRQ context (finally).

This last be could be interesting. One of the things i needed to do
was change request_irq to request_threaded_irq(). Because i need to do
MDIO reads to determine what PHY causes the interrupts, the code is in
thread context when it actually fires the interrupt.

The core IRQ code is happy to turn an interrupt context interrupt into
a threaded context interrupt, but it cannot do it the other way
around.

Using threaded interrupts would also allow the phylib code to be
simplified, but i didn't get that far yet.

	    Andrew