[RFC] if_spi.c: enable host interrupts after host ISR is registered

George Shore George.Shore at imgtec.com
Mon Oct 12 08:57:56 EDT 2009 

Hi Andrey,

> -----Original Message-----
> From: Andrey Yurovsky [mailto:andrey at cozybit.com]
> Sent: 09 October 2009 17:17
> To: George Shore
> Cc: libertas-dev at lists.infradead.org; Dan Williams
> Subject: Re: [RFC] if_spi.c: enable host interrupts after host ISR is
> registered
> 
> Hi George,
> 
> On Fri, Oct 9, 2009 at 3:37 AM, George Shore <George.Shore at imgtec.com>
> wrote:
> > During the firmware loading process a number of interrupts are fired
> > within this code. However, this happens when interrupts are being masked
> > out. When the host interrupts are enabled after the download an
> > interrupt is fired but left unhandled by the time the ISR is registered,
> > causing timeouts on every command until a warm reset is made. This issue
> > does not happen if the firmware code does not need to be downloaded.
> >
> > This moves the call to spu_set_interrupt_mode() to after the ISR is
> > registered, so that the interrupt caused by the firmware load can be
> > handled by the ISR (which currently does nothing with it anyway).
> >
> > Any thoughts upon this?
> 
> We haven't seen the command timeouts you're describing with the driver
> the way it is.  What's your setup like?
> 

I'm using the 8686 via gspi with on in-house hardware with an in-house CPU. I'm beginning to think the problem is more on our side of the fence...

> With your patch applied, I see timeouts:
> 
> [17179586.484000] libertas_spi: Libertas SPI driver
> [17179586.528000] libertas_spi spi0.0: firmware: requesting
> libertas/gspi8686_hn
> [17179587.640000] libertas_spi spi0.0: firmware: requesting
> libertas/gspi8686.bn
> [17179591.916000] libertas: command 0x0003 timed out
> [17179591.920000] libertas: requeueing command 0x0003 due to timeout (#1)
> [17179594.924000] libertas: command 0x0003 timed out
> [17179594.928000] libertas: requeueing command 0x0003 due to timeout (#2)
> [17179597.936000] libertas: command 0x0003 timed out
> [17179597.940000] libertas: requeueing command 0x0003 due to timeout (#3)
> [17179600.944000] libertas: command 0x0003 timed out
> [17179600.948000] libertas: Excessive timeouts submitting command 0x0003
> [17179600.952000] libertas: PREP_CMD: command 0x0003 failed: -110
> [17179600.960000] libertas_spi: probe of spi0.0 failed with error -110
> 

Yeah, I thought it was a little too good to be true to be changing bits of a device driver that works for other people to make it work for me :)

Without the patch, I was receiving:
[  329.087773] libertas_spi: Libertas SPI driver
[  330.154280] libertas_spi spi0.1: firmware: using built-in firmware libertas/gspi8686_hlp.bin
[  468.726590] libertas_spi spi0.1: firmware: using built-in firmware libertas/gspi8686.bin
[  526.401588] libertas: command 0x0003 timed out
[  526.426667] libertas: requeueing command 0x0003 due to timeout (#1)
[  526.466897] libertas: Received result 0 to command 3 after 1 retries
[  526.494199] libertas: 00:13:e0:b8:61:46, fw 9.70.3p37, cap 0x00000303
[  526.506927] libertas: unidentified region code; using the default (USA)
[  526.546953] libertas: Received CMD_RESP with invalid sequence 1 (expected 2)
[  529.531525] libertas: command 0x001e timed out
[  529.557658] libertas: requeueing command 0x001e due to timeout (#1)
[  529.595596] libertas: Received result 0 to command 1e after 1 retries
[  529.687728] libertas: Received CMD_RESP with invalid sequence 2 (expected 3)
[  532.671527] libertas: command 0x0028 timed out
[  532.698173] libertas: requeueing command 0x0028 due to timeout (#1)
[  532.736723] libertas: Received result 0 to command 28 after 1 retries
[  532.775889] libertas: Received CMD_RESP with invalid sequence 3 (expected 4)
[  535.761536] libertas: command 0x001d timed out
[  535.788422] libertas: requeueing command 0x001d due to timeout (#1)
[  535.829696] libertas: Received result 0 to command 1d after 1 retries
[  535.855839] libertas: wlan0: Marvell WLAN 802.11 adapter

This goes on until a warm reset (where the firmware is still loaded) and then everything is fine. I'm sure it's an interrupt issue, and the more I think about it the more I tend towards the interrupt code we have for the board being the cause...

> Also one minor issue with the patch,
> 
> > ---
> > drivers/net/wireless/libertas/if_spi.c | 11 +++++++----
> > 1 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/libertas/if_spi.c
> > b/drivers/net/wireless/libertas/if_spi.c
> > index 6564282..f95d9e8 100644
> > --- a/drivers/net/wireless/libertas/if_spi.c
> > +++ b/drivers/net/wireless/libertas/if_spi.c
> > @@ -1103,10 +1103,6 @@ static int __devinit if_spi_probe(struct
> > spi_device *spi)
> > lbs_deb_spi("loaded FW for Marvell WLAN 802.11 adapter\n");
> > }
> >
> > - err = spu_set_interrupt_mode(card, 0, 1);
> > - if (err)
> > - goto free_card;
> > -
> > /* Register our card with libertas.
> > * This will call alloc_etherdev */
> > priv = lbs_add_card(card, &spi->dev);
> > @@ -1139,6 +1135,13 @@ static int __devinit if_spi_probe(struct
> > spi_device *spi)
> > goto terminate_thread;
> > }
> >
> > + /* Enable the card->host interrupts now that the ISR has been
> > + * registered. This will allow the driver to handle the interrupts
> > + * that are fired when the firmware is loading. */
> > + err = spu_set_interrupt_mode(card, 0, 1);
> > + if (err)
> > + goto free_card;
> > +
> 
> If we fail here, we should "goto terminate_thread", "free_card" made
> sense where that line was originally located but we need to undo more
> if we fail later on as you have it now.
> 

Eeek... I knew there was something I'd forgotten to change. I blame the rush of adrenaline from submitting something to a mailing list ;)

> > /* Start the card.
> > * This will call register_netdev, and we'll start
> > * getting interrupts... */
> >
> > --
> > 1.6.5.rc2
> > -
> > This message is subject to Imagination Technologies' e-mail terms:
> http://www.imgtec.com/e-mail.htm
> >
> > Imagination Technologies Ltd is a limited company registered in England
> No:  1306335
> > Registered Office: Imagination House, Home Park Estate, Kings Langley,
> Hertfordshire, WD4 8LZ.
> >
> > Email to and from the company may be monitored for compliance and other
> administrative purposes.
> > -
> >
> >
> > _______________________________________________
> > libertas-dev mailing list
> > libertas-dev at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/libertas-dev
> >

Thanks for the feedback - I've think I should've asked questions first, THEN posted a patch suggestion! :)

Regards,
George
-
This message is subject to Imagination Technologies' e-mail terms: http://www.imgtec.com/e-mail.htm

Imagination Technologies Ltd is a limited company registered in England No:  1306335 
Registered Office: Imagination House, Home Park Estate, Kings Langley, Hertfordshire, WD4 8LZ.  

Email to and from the company may be monitored for compliance and other administrative purposes.  
-

More information about the libertas-dev mailing list