SDIO firmware download error
Dan Williams
dcbw at redhat.com
Tue Apr 6 12:19:19 EDT 2010
On Tue, 2010-04-06 at 09:04 -0700, Dan Williams wrote:
> On Tue, 2010-04-06 at 14:02 +0530, Alagu Sankar wrote:
> > On Tue, Apr 6, 2010 at 10:42 AM, Dan Williams <dcbw at redhat.com> wrote:
> > > On Mon, 2010-04-05 at 19:06 +0530, Alagu Sankar wrote:
> > >> On Thu, Apr 1, 2010 at 12:41 PM, Dan Williams <dcbw at redhat.com> wrote:
> > >> > On Wed, 2010-03-31 at 11:11 +0100, Jonathan Cameron wrote:
> > >> >> On 03/30/10 21:29, Dan Williams wrote:
> > >> >> > On Thu, 2010-03-11 at 13:49 +0530, Alagu Sankar wrote:
> > >> >> >> Trying my luck with a different subject as the previous one was rejected..
> > >> >> >>
> > >> >> >> I am in the process of adding SDIO Support for Davinci and using the
> > >> >> >> Libertas SDIO driver as my standard test setup. I am using the Linux
> > >> >> >> 2.6.33-rc6 Linux kernel and the associated libertas driver. I have
> > >> >> >> access to 8385, 8686 and 8688 cards from different vendors, with
> > >> >> >> diferent firmware versions. I would like to get some inputs from the
> > >> >> >> list for the following issue that I am facing.
> > >> >> >>
> > >> >> >> When I use the libertas driver as is, the helper download is always
> > >> >> >> successful, but the primary firmware loading fails after the first
> > >> >> >> transfer. I get a req_size of 17 after the initial 16 byte transfer,
> > >> >> >> indicating that the error bit is set. For a typical working setup,
> > >> >> >> there is a 16 byte transfer, followed by a 12 byte transfer and so on.
> > >> >> >> If I introduce a delay of 2 milli-seconds between each transfer of
> > >> >> >> the helper firmware, then there is no problem in downloading the
> > >> >> >> primary firmware. Even enabling the debug message
> > >> >> >> "lbs_deb_sdio("sending %d bytes chunk\n", chunk_size);" alone would
> > >> >> >> result in successful firmware download.
> > >> >> >
> > >> >> > If we can reproduce this issue on some other platform (to determine that
> > >> >> > the host controller is *not* the culprit) I'm not necessarily opposed to
> > >> >> > adding a small delay during helper firmware download in the driver. But
> > >> >> > beyond adding some debugging prints to the MMC layer to conclusively
> > >> >> > determine that the HC is responding correctly or incorrectly to the
> > >> >> > transfers, I'm not really sure. Are you aware of any errata for your
> > >> >> > SDIO controller that might affect this?
> > >> >> A similar problem exists on a pxa271. I'm carrying the following patch
> > >> >> for it... Note this is against a fairly old kernel, 2.6.32-rc6
> > >> >>
> > >> >> Symptoms were exactly the same though. There was a previous thread on this
> > >> >> list about it:
> > >> >>
> > >> >> sdio 8686 firmware load problem
> > >> >>
> > >> >> back on the 11th of August last year.
> > >> >
> > >> > Any chance if you see the issue you could make the mdelay() just a few
> > >> > lines higher inside the while (1) loop into mdelay(2) ? If that doesn't
> > >> > work, move the mdelay(1) in the while(1) loop up somewhere above the "if
> > >> > ((status & IF_SDIO_IO_RDY) &&" part.
> > >> >
> > >> > Dan
> > >> >
> > >>
> > >> Tried increasing the mdelay(1) upto mdelay(5) and it did not help. But
> > >> moving the mdelay(1) above the "if ((status & IF_SDIO_IO_RDY) &&" part
> > >> and increasing it to mdelay(2) resolved the issues on Davinci
> > >> platforms.
> > >
> > > Can you test the following patch for me? It works on both a Marvell
> > > 8686 developer board and the Wi2Wi 8686 board with my Ricoh laptop SD
> > > controller (from which I'm writing this mail).
> > >
> > > The fact that the delay worked for you there means that either the
> > > firmware or your SDHC needs (a) a short delay before banging the STATUS
> > > register, and/or (b) a short delay after banging that status register
> > > before trying to write a packet to the card. Or something like that.
> > >
> > > Let me know if the patch works and if so I'll forward it to John.
> > >
> > > Dan
> > > ---
> > >
> > > diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> > > index 7a73f62..61a46df 100644
> > > --- a/drivers/net/wireless/libertas/if_sdio.c
> > > +++ b/drivers/net/wireless/libertas/if_sdio.c
> > > @@ -312,12 +312,33 @@ out:
> > > return ret;
> > > }
> > >
> > > +static int if_sdio_wait_status(struct if_sdio_card *card, const u8 condition)
> > > +{
> > > + u8 status;
> > > + unsigned long timeout;
> > > + int ret = 0;
> > > +
> > > + timeout = jiffies + HZ;
> > > + while (1) {
> > > + status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
> > > +
> > > + /* Short delay between reads which some platforms and firmware
> > > + * versions (like Davinci) seem to need.
> > > + */
> > > + mdelay(2);
> > > +
> > > + if (ret || (status & condition))
> > > + break;
> > > + if (time_after(jiffies, timeout))
> > > + return -ETIMEDOUT;
> > > + }
> > > + return ret;
> > > +}
> > > +
> > > static int if_sdio_card_to_host(struct if_sdio_card *card)
> > > {
> > > int ret;
> > > - u8 status;
> > > u16 size, type, chunk;
> > > - unsigned long timeout;
> > >
> > > lbs_deb_enter(LBS_DEB_SDIO);
> > >
> > > @@ -332,19 +353,9 @@ static int if_sdio_card_to_host(struct if_sdio_card *card)
> > > goto out;
> > > }
> > >
> > > - timeout = jiffies + HZ;
> > > - while (1) {
> > > - status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
> > > - if (ret)
> > > - goto out;
> > > - if (status & IF_SDIO_IO_RDY)
> > > - break;
> > > - if (time_after(jiffies, timeout)) {
> > > - ret = -ETIMEDOUT;
> > > - goto out;
> > > - }
> > > - mdelay(1);
> > > - }
> > > + ret = if_sdio_wait_status(card, IF_SDIO_IO_RDY);
> > > + if (ret)
> > > + goto out;
> > >
> > > /*
> > > * The transfer must be in one transaction or the firmware
> > > @@ -411,8 +422,6 @@ static void if_sdio_host_to_card_worker(struct work_struct *work)
> > > {
> > > struct if_sdio_card *card;
> > > struct if_sdio_packet *packet;
> > > - unsigned long timeout;
> > > - u8 status;
> > > int ret;
> > > unsigned long flags;
> > >
> > > @@ -432,25 +441,15 @@ static void if_sdio_host_to_card_worker(struct work_struct *work)
> > >
> > > sdio_claim_host(card->func);
> > >
> > > - timeout = jiffies + HZ;
> > > - while (1) {
> > > - status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
> > > - if (ret)
> > > - goto release;
> > > - if (status & IF_SDIO_IO_RDY)
> > > - break;
> > > - if (time_after(jiffies, timeout)) {
> > > - ret = -ETIMEDOUT;
> > > - goto release;
> > > - }
> > > - mdelay(1);
> > > + ret = if_sdio_wait_status(card, IF_SDIO_IO_RDY);
> > > + if (ret == 0) {
> > > + ret = sdio_writesb(card->func, card->ioport,
> > > + packet->buffer, packet->nb);
> > > }
> > >
> > > - ret = sdio_writesb(card->func, card->ioport,
> > > - packet->buffer, packet->nb);
> > > if (ret)
> > > - goto release;
> > > -release:
> > > + lbs_pr_err("error %d sending packet to firmware\n", ret);
> > > +
> > > sdio_release_host(card->func);
> > >
> > > kfree(packet);
> > > @@ -463,10 +462,11 @@ release:
> > > /* Firmware */
> > > /********************************************************************/
> > >
> > > +#define FW_DL_READY_STATUS (IF_SDIO_IO_RDY | IF_SDIO_DL_RDY)
> > > +
> > > static int if_sdio_prog_helper(struct if_sdio_card *card)
> > > {
> > > int ret;
> > > - u8 status;
> > > const struct firmware *fw;
> > > unsigned long timeout;
> > > u8 *chunk_buffer;
> > > @@ -498,20 +498,9 @@ static int if_sdio_prog_helper(struct if_sdio_card *card)
> > > size = fw->size;
> > >
> > > while (size) {
> > > - timeout = jiffies + HZ;
> > > - while (1) {
> > > - status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
> > > - if (ret)
> > > - goto release;
> > > - if ((status & IF_SDIO_IO_RDY) &&
> > > - (status & IF_SDIO_DL_RDY))
> > > - break;
> > > - if (time_after(jiffies, timeout)) {
> > > - ret = -ETIMEDOUT;
> > > - goto release;
> > > - }
> > > - mdelay(1);
> > > - }
> > > + ret = if_sdio_wait_status(card, FW_DL_READY_STATUS);
> > > + if (ret)
> > > + goto release;
> > >
> > > chunk_size = min(size, (size_t)60);
> > >
> > > @@ -581,7 +570,6 @@ out:
> > > static int if_sdio_prog_real(struct if_sdio_card *card)
> > > {
> > > int ret;
> > > - u8 status;
> > > const struct firmware *fw;
> > > unsigned long timeout;
> > > u8 *chunk_buffer;
> > > @@ -613,20 +601,9 @@ static int if_sdio_prog_real(struct if_sdio_card *card)
> > > size = fw->size;
> > >
> > > while (size) {
> > > - timeout = jiffies + HZ;
> > > - while (1) {
> > > - status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
> > > - if (ret)
> > > - goto release;
> > > - if ((status & IF_SDIO_IO_RDY) &&
> > > - (status & IF_SDIO_DL_RDY))
> > > - break;
> > > - if (time_after(jiffies, timeout)) {
> > > - ret = -ETIMEDOUT;
> > > - goto release;
> > > - }
> > > - mdelay(1);
> > > - }
> > > + ret = if_sdio_wait_status(card, FW_DL_READY_STATUS);
> > > + if (ret)
> > > + goto release;
> > >
> > > req_size = sdio_readb(card->func, IF_SDIO_RD_BASE, &ret);
> > > if (ret)
> > >
> > >
> > >
> >
> > The patch works fine, but throughput is reduced drastically, as the
> > delay becomes common for all transfers (if_sdio_host_to_card and
> > if_sdio_card_to_host). We need the delay only during the helper
> > firmware download. It is not required for the real firmware download
> > or any other data transfers. Though this patch results in a far
> > better approach for status checking, without knowing the root cause of
> > the problem, it may be a better option to leave the code as is, with
> > the mdelay introduced only for helper firmware loading code.
Can you try this version? Basically we're just playing around with
where the mdelay sits. This patch will help us determine whether the
additional delay is required between reads of the IF_SDIO_STATUS
register during helper download, or whether it's required between a read
of IF_SDIO_STATUS and sending the first block.
Dan
---
diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 7a73f62..72174d1 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -312,12 +312,28 @@ out:
return ret;
}
+static int if_sdio_wait_status(struct if_sdio_card *card, const u8 condition)
+{
+ u8 status;
+ unsigned long timeout;
+ int ret = 0;
+
+ timeout = jiffies + HZ;
+ while (1) {
+ status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
+ if (ret || (status & condition))
+ break;
+ if (time_after(jiffies, timeout))
+ return -ETIMEDOUT;
+ mdelay(1);
+ }
+ return ret;
+}
+
static int if_sdio_card_to_host(struct if_sdio_card *card)
{
int ret;
- u8 status;
u16 size, type, chunk;
- unsigned long timeout;
lbs_deb_enter(LBS_DEB_SDIO);
@@ -332,19 +348,9 @@ static int if_sdio_card_to_host(struct if_sdio_card *card)
goto out;
}
- timeout = jiffies + HZ;
- while (1) {
- status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
- if (ret)
- goto out;
- if (status & IF_SDIO_IO_RDY)
- break;
- if (time_after(jiffies, timeout)) {
- ret = -ETIMEDOUT;
- goto out;
- }
- mdelay(1);
- }
+ ret = if_sdio_wait_status(card, IF_SDIO_IO_RDY);
+ if (ret)
+ goto out;
/*
* The transfer must be in one transaction or the firmware
@@ -411,8 +417,6 @@ static void if_sdio_host_to_card_worker(struct work_struct *work)
{
struct if_sdio_card *card;
struct if_sdio_packet *packet;
- unsigned long timeout;
- u8 status;
int ret;
unsigned long flags;
@@ -432,25 +436,15 @@ static void if_sdio_host_to_card_worker(struct work_struct *work)
sdio_claim_host(card->func);
- timeout = jiffies + HZ;
- while (1) {
- status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
- if (ret)
- goto release;
- if (status & IF_SDIO_IO_RDY)
- break;
- if (time_after(jiffies, timeout)) {
- ret = -ETIMEDOUT;
- goto release;
- }
- mdelay(1);
+ ret = if_sdio_wait_status(card, IF_SDIO_IO_RDY);
+ if (ret == 0) {
+ ret = sdio_writesb(card->func, card->ioport,
+ packet->buffer, packet->nb);
}
- ret = sdio_writesb(card->func, card->ioport,
- packet->buffer, packet->nb);
if (ret)
- goto release;
-release:
+ lbs_pr_err("error %d sending packet to firmware\n", ret);
+
sdio_release_host(card->func);
kfree(packet);
@@ -463,10 +457,11 @@ release:
/* Firmware */
/********************************************************************/
+#define FW_DL_READY_STATUS (IF_SDIO_IO_RDY | IF_SDIO_DL_RDY)
+
static int if_sdio_prog_helper(struct if_sdio_card *card)
{
int ret;
- u8 status;
const struct firmware *fw;
unsigned long timeout;
u8 *chunk_buffer;
@@ -498,20 +493,14 @@ static int if_sdio_prog_helper(struct if_sdio_card *card)
size = fw->size;
while (size) {
- timeout = jiffies + HZ;
- while (1) {
- status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
- if (ret)
- goto release;
- if ((status & IF_SDIO_IO_RDY) &&
- (status & IF_SDIO_DL_RDY))
- break;
- if (time_after(jiffies, timeout)) {
- ret = -ETIMEDOUT;
- goto release;
- }
- mdelay(1);
- }
+ ret = if_sdio_wait_status(card, FW_DL_READY_STATUS);
+ if (ret)
+ goto release;
+
+ /* On some platforms (like Davinci) the chip needs more time
+ * between helper blocks.
+ */
+ mdelay(1);
chunk_size = min(size, (size_t)60);
@@ -581,7 +570,6 @@ out:
static int if_sdio_prog_real(struct if_sdio_card *card)
{
int ret;
- u8 status;
const struct firmware *fw;
unsigned long timeout;
u8 *chunk_buffer;
@@ -613,20 +601,9 @@ static int if_sdio_prog_real(struct if_sdio_card *card)
size = fw->size;
while (size) {
- timeout = jiffies + HZ;
- while (1) {
- status = sdio_readb(card->func, IF_SDIO_STATUS, &ret);
- if (ret)
- goto release;
- if ((status & IF_SDIO_IO_RDY) &&
- (status & IF_SDIO_DL_RDY))
- break;
- if (time_after(jiffies, timeout)) {
- ret = -ETIMEDOUT;
- goto release;
- }
- mdelay(1);
- }
+ ret = if_sdio_wait_status(card, FW_DL_READY_STATUS);
+ if (ret)
+ goto release;
req_size = sdio_readb(card->func, IF_SDIO_RD_BASE, &ret);
if (ret)
More information about the libertas-dev
mailing list