Re: wcnss_ctrl firmware loading

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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