SDIO firmware download error

Alagu Sankar alagusankar at gmail.com
Tue Apr 6 04:32:33 EDT 2010


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.

- Alagu Sankar



More information about the libertas-dev mailing list