SDIO firmware download error

Dan Williams dcbw at redhat.com
Wed Apr 14 14:43:54 EDT 2010


On Tue, 2010-04-13 at 16:53 +0530, Alagu Sankar wrote:
> On Tue, Apr 13, 2010 at 12:56 AM, Dan Williams <dcbw at redhat.com> wrote:
> > On Tue, 2010-04-06 at 09:19 -0700, Dan Williams wrote:
> >> 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.
> >
> > Any luck with this latest patch?
> >
> > Thanks,
> > Dan
> >
> >> 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)
> >>
> >>
> >>
> >> _______________________________________________
> >> libertas-dev mailing list
> >> libertas-dev at lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/libertas-dev
> >
> >
> >
> 
> This patch works with "mdelay(2)" instead of "mdelay(1)".  With
> mdelay(1) in helper firmware, it works if we keep all the modules
> loaded and then insert the card.  If we insert the card first and then
> load the modules, then it fails.  With mdelay(2), it works for all
> conditions.

Do you mean the mdelay(1) in if_sdio_prog_helper(), or the  mdelay(1) in
if_sdio_wait_status()?

Dan




More information about the libertas-dev mailing list