SDIO firmware download error
Alagu Sankar
alagusankar at gmail.com
Thu Apr 15 14:35:48 EDT 2010
On Thu, Apr 15, 2010 at 12:13 AM, Dan Williams <dcbw at redhat.com> wrote:
> 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
>
>
The mdelay(2) is required in if_sdio_prog_helper().
- Alagu Sankar
More information about the libertas-dev
mailing list