Message ID | 9235D6609DB808459E95D78E17F2E43D40493795@CHN-SV-EXMX02.mchp-main.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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
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.
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
> 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
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
> 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
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.
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
> 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
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
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(-)