SDIO firmware download error
Dan Williams
dcbw at redhat.com
Tue Apr 6 12:04:43 EDT 2010
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.
Ok, will respin.
Dan
More information about the libertas-dev
mailing list