phylib interrupt question

phylib interrupt question

[Steve.Glendinning]

Hi everybody,

I'm converting an ethernet driver to use phylib, and I have an interrupt 
question.  My device is an integrated mac/phy, with the option to disable 
the internal phy and use an external phy instead.  I've succesfully used 
phylib in polling mode, but I'd like to convert this to be 
interrupt-driven if possible.

When using the internal phy, it can raise interrupts via the mac (by 
setting a "phy interrupt" bit in the mac's interrupt status register).  Is 
there a way of signalling this phy interrupt to phylib from my mac 
driver's isr?  Phylib seems to want to connect to a real irq?

Regards,
--
Steve Glendinning
SMSC GmbH
m: +44 777 933 9124
e: steve.glendinning@smsc.com

Re: phylib interrupt question

[Andy Fleming]

On Jun 6, 2008, at 11:07, steve.glendinning@smsc.com wrote:

> Hi everybody,
>
> I'm converting an ethernet driver to use phylib, and I have an  
> interrupt
> question.  My device is an integrated mac/phy, with the option to  
> disable
> the internal phy and use an external phy instead.  I've succesfully  
> used
> phylib in polling mode, but I'd like to convert this to be
> interrupt-driven if possible.
>
> When using the internal phy, it can raise interrupts via the mac (by
> setting a "phy interrupt" bit in the mac's interrupt status  
> register).  Is
> there a way of signalling this phy interrupt to phylib from my mac
> driver's isr?  Phylib seems to want to connect to a real irq?

Hm.  Looks like one of the features of Phylib got accidentally  
dropped.  However, it's a simple 2-line fix which I'm about to submit.

The Phylib supports *three* methods of link state detection:

1) The Phylib handles the irq.  To get this functionality, you set  
phydev->irq to the irq number.  Obviously, this doesn't work if your  
PHY is sending interrupts through your MAC's interrupt.  You don't  
want your MAC's interrupts disabled while the PHY is being managed!

2) The Phylib polls the PHY regularly.  To get this, you set phydev- 
 >irq to PHY_POLL.  Obvious deficiencies, but it's easiest

3) Something else handles the irq, and then tells the Phylib to check  
the state.  To get this, you set phydev->irq to PHY_IGNORE_INTERRUPT.   
Sadly, this requires that we not assume that !PHY_POLL is the same as  
option #1, which in 2 instances, we forgot to do.  I am sending a  
patch right after this to fix that.

Then, in your interrupt handler, you do something like this:

if (interrupt_cause & PHY_INTERRUPT) {
	err = phy_disable_interrupts(phydev);
	if (err)
		goto phy_err;

	mutex_lock(&phydev->lock);
	 if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
		phydev->state = PHY_CHANGELINK;
	mutex_unlock(&phydev->lock);

	// clear PHY_INTERRUPT bit
}

This will tell the Phylib that an interrupt occurred, and it will read  
the link status in the normal fashion.

Clearly, you will be the first to blaze this trail, so let us know if  
there are any problems, since one of the goals was for phylib to  
support that functionality.

Andy

Re: phylib interrupt question

[Andy Fleming]

On Jun 6, 2008, at 12:11, Andy Fleming wrote:

>
> On Jun 6, 2008, at 11:07, steve.glendinning@smsc.com wrote:
>
>> Hi everybody,
>>
>> I'm converting an ethernet driver to use phylib, and I have an  
>> interrupt
>> question.  My device is an integrated mac/phy, with the option to  
>> disable
>> the internal phy and use an external phy instead.  I've succesfully  
>> used
>> phylib in polling mode, but I'd like to convert this to be
>> interrupt-driven if possible.
>>
>> When using the internal phy, it can raise interrupts via the mac (by
>> setting a "phy interrupt" bit in the mac's interrupt status  
>> register).  Is
>> there a way of signalling this phy interrupt to phylib from my mac
>> driver's isr?  Phylib seems to want to connect to a real irq?
>
> Hm.  Looks like one of the features of Phylib got accidentally  
> dropped.  However, it's a simple 2-line fix which I'm about to submit.


D'oh, I'm totally wrong about that being a bug.  Both of the places  
that check phydev->irq != PHY_POLL are valid.  Good news is: that  
means you don't need to wait for a patch!

Andy

Re: phylib interrupt question

[Trent Piepho]

On Fri, 6 Jun 2008, Andy Fleming wrote:
> The Phylib supports *three* methods of link state detection:
>
> 1) The Phylib handles the irq.  To get this functionality, you set 
> phydev->irq to the irq number.  Obviously, this doesn't work if your PHY is 
> sending interrupts through your MAC's interrupt.  You don't want your MAC's 
> interrupts disabled while the PHY is being managed!

In my quest to decrease boot times, I've found something that seems odd about
the way phylib handles the phy IRQ.

phylib has a work queue, state_queue, that runs one per second
(PHY_STATE_TIME), in interrupt mode and polling mode.  In polling mode it
checks the phy status every other time it runs.  In interrupt mode only when
it's told.  Which makes me wonder if it really needs to always run once per
second in interrupt mode, but that's a different issue.

When a phy interrupt occurs, a different work queue, phy_queue which runs the
function phy_change(), is run.  It disables the irq, sets the phy state
machine to "changed", and re-enables the irq.  It's not clear to me why
interrupts need to be disabled, but that's not my point either.

My issue is that the code triggered by the irq never schedules the state_queue
work queue.  It sets the phy's state to changed, but doesn't actually _do_
anything.  The MAC, net layer, userspace, etc.  don't find out the link went
up or down until state_queue's timer runs the queue one second layer.

If you increase PHY_STATE_TIME to 10 seconds and un-plug/re-plug the cable,
you can see that it takes some time before the kernel responds to the link
change.  If you need to run DHCP to get your device online, then this waiting
time gets added to your boot time.

It seems like this simple patch is all that's needed to fix it:
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -728,6 +728,7 @@ static void phy_change(struct work_struct *work)
  	if (err)
 		goto irq_enable_err;

+	schedule_work(&phydev->state_queue);
  	return;

  irq_enable_err:


So is there any reason not to do this?