wcnss_ctrl firmware loading

wcnss_ctrl firmware loading

[Will Newton]

Hi,

I'm running into some issues with getting this driver running on an
msm8909. The firmware loading code looks a little odd to me so I would
just like to check it is doing the right thing:



                               data = fw->data;
        left = fw->size;

        req->hdr.type = WCNSS_DOWNLOAD_NV_REQ;
        req->hdr.len = sizeof(*req) + NV_FRAGMENT_SIZE;

        req->last = 0;
        req->frag_size = NV_FRAGMENT_SIZE;

        req->seq = 0;
        do {
                if (left <= NV_FRAGMENT_SIZE) {
                        req->last = 1;
                        req->frag_size = left;
                        req->hdr.len = sizeof(*req) + left;
                }

                memcpy(req->fragment, data, req->frag_size);

                ret = rpmsg_send(wcnss->channel, req, req->hdr.len);
                if (ret < 0) {
                        dev_err(wcnss->dev, "failed to send smd packet\n");
                        goto release_fw;
                }

                /* Increment for next fragment */
                req->seq++;

                data += req->hdr.len;
                left -= NV_FRAGMENT_SIZE;
        } while (left > 0);

data is incremented by req->hdr.len which includes sizeof(*req) which
seems wrong, e.g. it should perhaps increment by req->frag_size. Or am
I missing some subtlety here?

Strangely it doesn't seem to matter if I change that code, either way
my firmware startup fails...

Re: wcnss_ctrl firmware loading

[Mario Kicherer]

Hi,

at least the driver in this Android kernel also only increases by
NV_FRAGMENT_SIZE:

https://github.com/anyc/linux-zenwatch3/blob/master/drivers/net/wireless/wcnss/wcnss_wlan.c#L2413

Best regards,
Mario

On 04.01.2018 11:44, Will Newton wrote:
> Hi,
> 
> I'm running into some issues with getting this driver running on an
> msm8909. The firmware loading code looks a little odd to me so I would
> just like to check it is doing the right thing:
> 
> 
> 
>                                 data = fw->data;
>          left = fw->size;
> 
>          req->hdr.type = WCNSS_DOWNLOAD_NV_REQ;
>          req->hdr.len = sizeof(*req) + NV_FRAGMENT_SIZE;
> 
>          req->last = 0;
>          req->frag_size = NV_FRAGMENT_SIZE;
> 
>          req->seq = 0;
>          do {
>                  if (left <= NV_FRAGMENT_SIZE) {
>                          req->last = 1;
>                          req->frag_size = left;
>                          req->hdr.len = sizeof(*req) + left;
>                  }
> 
>                  memcpy(req->fragment, data, req->frag_size);
> 
>                  ret = rpmsg_send(wcnss->channel, req, req->hdr.len);
>                  if (ret < 0) {
>                          dev_err(wcnss->dev, "failed to send smd packet\n");
>                          goto release_fw;
>                  }
> 
>                  /* Increment for next fragment */
>                  req->seq++;
> 
>                  data += req->hdr.len;
>                  left -= NV_FRAGMENT_SIZE;
>          } while (left > 0);
> 
> data is incremented by req->hdr.len which includes sizeof(*req) which
> seems wrong, e.g. it should perhaps increment by req->frag_size. Or am
> I missing some subtlety here?
> 
> Strangely it doesn't seem to matter if I change that code, either way
> my firmware startup fails...
> 

Re: wcnss_ctrl firmware loading

[Will Newton]

On Thu, Jan 4, 2018 at 11:36 AM, Mario Kicherer <dev@kicherer.org> wrote:
> Hi,
>
> at least the driver in this Android kernel also only increases by
> NV_FRAGMENT_SIZE:
>
> https://github.com/anyc/linux-zenwatch3/blob/master/drivers/net/wireless/wcnss/wcnss_wlan.c#L2413

It also appears to skip the first 4 bytes of the firmware, but again I
tried this with my board and still no successful firmware boot, so I
don't know if the change makes any material difference...

> On 04.01.2018 11:44, Will Newton wrote:
>>
>> Hi,
>>
>> I'm running into some issues with getting this driver running on an
>> msm8909. The firmware loading code looks a little odd to me so I would
>> just like to check it is doing the right thing:
>>
>>
>>
>>                                 data = fw->data;
>>          left = fw->size;
>>
>>          req->hdr.type = WCNSS_DOWNLOAD_NV_REQ;
>>          req->hdr.len = sizeof(*req) + NV_FRAGMENT_SIZE;
>>
>>          req->last = 0;
>>          req->frag_size = NV_FRAGMENT_SIZE;
>>
>>          req->seq = 0;
>>          do {
>>                  if (left <= NV_FRAGMENT_SIZE) {
>>                          req->last = 1;
>>                          req->frag_size = left;
>>                          req->hdr.len = sizeof(*req) + left;
>>                  }
>>
>>                  memcpy(req->fragment, data, req->frag_size);
>>
>>                  ret = rpmsg_send(wcnss->channel, req, req->hdr.len);
>>                  if (ret < 0) {
>>                          dev_err(wcnss->dev, "failed to send smd
>> packet\n");
>>                          goto release_fw;
>>                  }
>>
>>                  /* Increment for next fragment */
>>                  req->seq++;
>>
>>                  data += req->hdr.len;
>>                  left -= NV_FRAGMENT_SIZE;
>>          } while (left > 0);
>>
>> data is incremented by req->hdr.len which includes sizeof(*req) which
>> seems wrong, e.g. it should perhaps increment by req->frag_size. Or am
>> I missing some subtlety here?
>>
>> Strangely it doesn't seem to matter if I change that code, either way
>> my firmware startup fails...
>>
>

Re: wcnss_ctrl firmware loading

[Bjorn Andersson]

On Thu 04 Jan 02:44 PST 2018, Will Newton wrote:

> Hi,
> 
> I'm running into some issues with getting this driver running on an
> msm8909. The firmware loading code looks a little odd to me so I would
> just like to check it is doing the right thing:
> 

Sorry for not seeing your mail earlier Will.

> 
> 
>                                data = fw->data;
>         left = fw->size;
> 
>         req->hdr.type = WCNSS_DOWNLOAD_NV_REQ;
>         req->hdr.len = sizeof(*req) + NV_FRAGMENT_SIZE;
> 
>         req->last = 0;
>         req->frag_size = NV_FRAGMENT_SIZE;
> 
>         req->seq = 0;
>         do {
>                 if (left <= NV_FRAGMENT_SIZE) {
>                         req->last = 1;
>                         req->frag_size = left;
>                         req->hdr.len = sizeof(*req) + left;
>                 }
> 
>                 memcpy(req->fragment, data, req->frag_size);
> 
>                 ret = rpmsg_send(wcnss->channel, req, req->hdr.len);
>                 if (ret < 0) {
>                         dev_err(wcnss->dev, "failed to send smd packet\n");
>                         goto release_fw;
>                 }
> 
>                 /* Increment for next fragment */
>                 req->seq++;
> 
>                 data += req->hdr.len;

You're correct, this will skip 16 bytes of the firmware for every
fragment of 3072 bytes. I would expect that the download command would
notify us about this, but apparently not.

I still don't know what this NV blob does though, because you have to
upload it again to bring up WiFi (which the wcn36xx driver does).

>                 left -= NV_FRAGMENT_SIZE;
>         } while (left > 0);
> 
> data is incremented by req->hdr.len which includes sizeof(*req) which
> seems wrong, e.g. it should perhaps increment by req->frag_size. Or am
> I missing some subtlety here?
> 
> Strangely it doesn't seem to matter if I change that code, either way
> my firmware startup fails...

I'm unable to see any behavioral change on the db410c after fixing this
as well. But the code is wrong.


The issues that I remember having while working on wcnss was that it
will crash either if the SMD links aren't there, if the pinctrl for the
wcnss pins wasn't set correctly or if the iris configuration
(wcnss_configure_iris()) wasn't done - and make sure that you have the
right XO clock enabled.

As you have SMD I would recommend that you double check the two latter
pieces.

Regards,
Bjorn