Re: [PATCH 4/5] soc: qcom: wcnss_ctrl: Make wcnss_ctrl parent the other components

2016-03-31 Thread Bjorn Andersson 
On Thu 31 Mar 11:47 PDT 2016, Andy Gross wrote:

> On Mon, Mar 28, 2016 at 09:35:25PM -0700, Bjorn Andersson wrote:
> > We need the signal from wcnss_ctrl indicating that the firmware is up
> > and running before we can communicate with the other components of the
> > chip. So make these other components children of the wcnss_ctrl device,
> > so they can be probed in order.
> > 
> > The process seems to take between 1/2-5 seconds, so this is done in a
> > worker, instead of holding up the probe.
> > 
> > Also adding the wait for a cbc completion if the firmware indicates this is
> > needed - like on 8016.
> 
> Can you define what a CBC is?
> 

Unfortunately I have no clue and have not yet seen this abbreviation in
any documentation...

> 
> 
> 
> >  /**
> >   * wcnss_download_nv() - send nv binary to WCNSS
> > - * @work:  work struct to acquire wcnss context
> > + * @wcnss: wcnss_ctrl state handle
> > + *
> > + * Returns 1 on success or 2 to indicate upcoming cbc completion. Negative
> 
> maybe a nit, but define the return values
> 

I'll remap them a step down, so the function returns 0 on success and 1
on "need to wait for cbc complete". That way we don't have an undefined
hole in the return set as well.

> > + * errno on failure.
> >   */
> > -static void wcnss_download_nv(struct work_struct *work)
> > +static int wcnss_download_nv(struct wcnss_ctrl *wcnss)
> >  {
> > -   struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> > download_nv_work);
> > struct wcnss_download_nv_req *req;
> > const struct firmware *fw;
> > const void *data;
> > @@ -178,7 +207,7 @@ static void wcnss_download_nv(struct work_struct *work)
> >  
> > req = kzalloc(sizeof(*req) + NV_FRAGMENT_SIZE, GFP_KERNEL);
> > if (!req)
> > -   return;
> > +   return -ENOMEM;
> >  
> > ret = request_firmware(, NVBIN_FILE, wcnss->dev);
> > if (ret) {
> 
> 
> 
> > +static void wcnss_async_probe(struct work_struct *work)
> > +{
> > +   struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> > probe_work);
> > +   int ret;
> > +
> > +   ret = wcnss_request_version(wcnss);
> > +   if (ret < 0)
> > +   return;
> > +
> > +   ret = wcnss_download_nv(wcnss);
> > +   if (ret < 0)
> > +   return;
> > +
> > +   /* Wait for pending CBC completion if download nv returns 2 */
> > +   if (ret == 2) {
> 
> Perhaps have a #define for the value is warranted
> 

With the above remap I can rename ret here to need_cbc and the code will
be understandable.

> > +   ret = wait_for_completion_timeout(>cbc, 
> > WCNSS_REQUEST_TIMEOUT);
> > +   if (!ret)
> > +   dev_err(wcnss->dev, "expected cbc completion\n");
> > +   }
> > +
> > +   of_platform_populate(wcnss->dev->of_node, NULL, NULL, wcnss->dev);
> >  }
> 
> 

Thanks for the feedback!

Regards,
Bjorn

Re: [PATCH 4/5] soc: qcom: wcnss_ctrl: Make wcnss_ctrl parent the other components

2016-03-31 Thread Bjorn Andersson 
On Thu 31 Mar 11:47 PDT 2016, Andy Gross wrote:

> On Mon, Mar 28, 2016 at 09:35:25PM -0700, Bjorn Andersson wrote:
> > We need the signal from wcnss_ctrl indicating that the firmware is up
> > and running before we can communicate with the other components of the
> > chip. So make these other components children of the wcnss_ctrl device,
> > so they can be probed in order.
> > 
> > The process seems to take between 1/2-5 seconds, so this is done in a
> > worker, instead of holding up the probe.
> > 
> > Also adding the wait for a cbc completion if the firmware indicates this is
> > needed - like on 8016.
> 
> Can you define what a CBC is?
> 

Unfortunately I have no clue and have not yet seen this abbreviation in
any documentation...

> 
> 
> 
> >  /**
> >   * wcnss_download_nv() - send nv binary to WCNSS
> > - * @work:  work struct to acquire wcnss context
> > + * @wcnss: wcnss_ctrl state handle
> > + *
> > + * Returns 1 on success or 2 to indicate upcoming cbc completion. Negative
> 
> maybe a nit, but define the return values
> 

I'll remap them a step down, so the function returns 0 on success and 1
on "need to wait for cbc complete". That way we don't have an undefined
hole in the return set as well.

> > + * errno on failure.
> >   */
> > -static void wcnss_download_nv(struct work_struct *work)
> > +static int wcnss_download_nv(struct wcnss_ctrl *wcnss)
> >  {
> > -   struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> > download_nv_work);
> > struct wcnss_download_nv_req *req;
> > const struct firmware *fw;
> > const void *data;
> > @@ -178,7 +207,7 @@ static void wcnss_download_nv(struct work_struct *work)
> >  
> > req = kzalloc(sizeof(*req) + NV_FRAGMENT_SIZE, GFP_KERNEL);
> > if (!req)
> > -   return;
> > +   return -ENOMEM;
> >  
> > ret = request_firmware(, NVBIN_FILE, wcnss->dev);
> > if (ret) {
> 
> 
> 
> > +static void wcnss_async_probe(struct work_struct *work)
> > +{
> > +   struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> > probe_work);
> > +   int ret;
> > +
> > +   ret = wcnss_request_version(wcnss);
> > +   if (ret < 0)
> > +   return;
> > +
> > +   ret = wcnss_download_nv(wcnss);
> > +   if (ret < 0)
> > +   return;
> > +
> > +   /* Wait for pending CBC completion if download nv returns 2 */
> > +   if (ret == 2) {
> 
> Perhaps have a #define for the value is warranted
> 

With the above remap I can rename ret here to need_cbc and the code will
be understandable.

> > +   ret = wait_for_completion_timeout(>cbc, 
> > WCNSS_REQUEST_TIMEOUT);
> > +   if (!ret)
> > +   dev_err(wcnss->dev, "expected cbc completion\n");
> > +   }
> > +
> > +   of_platform_populate(wcnss->dev->of_node, NULL, NULL, wcnss->dev);
> >  }
> 
> 

Thanks for the feedback!

Regards,
Bjorn

Re: [PATCH 4/5] soc: qcom: wcnss_ctrl: Make wcnss_ctrl parent the other components

2016-03-31 Thread Andy Gross 
On Mon, Mar 28, 2016 at 09:35:25PM -0700, Bjorn Andersson wrote:
> We need the signal from wcnss_ctrl indicating that the firmware is up
> and running before we can communicate with the other components of the
> chip. So make these other components children of the wcnss_ctrl device,
> so they can be probed in order.
> 
> The process seems to take between 1/2-5 seconds, so this is done in a
> worker, instead of holding up the probe.
> 
> Also adding the wait for a cbc completion if the firmware indicates this is
> needed - like on 8016.

Can you define what a CBC is?




>  /**
>   * wcnss_download_nv() - send nv binary to WCNSS
> - * @work:work struct to acquire wcnss context
> + * @wcnss:   wcnss_ctrl state handle
> + *
> + * Returns 1 on success or 2 to indicate upcoming cbc completion. Negative

maybe a nit, but define the return values

> + * errno on failure.
>   */
> -static void wcnss_download_nv(struct work_struct *work)
> +static int wcnss_download_nv(struct wcnss_ctrl *wcnss)
>  {
> - struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> download_nv_work);
>   struct wcnss_download_nv_req *req;
>   const struct firmware *fw;
>   const void *data;
> @@ -178,7 +207,7 @@ static void wcnss_download_nv(struct work_struct *work)
>  
>   req = kzalloc(sizeof(*req) + NV_FRAGMENT_SIZE, GFP_KERNEL);
>   if (!req)
> - return;
> + return -ENOMEM;
>  
>   ret = request_firmware(, NVBIN_FILE, wcnss->dev);
>   if (ret) {



> +static void wcnss_async_probe(struct work_struct *work)
> +{
> + struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> probe_work);
> + int ret;
> +
> + ret = wcnss_request_version(wcnss);
> + if (ret < 0)
> + return;
> +
> + ret = wcnss_download_nv(wcnss);
> + if (ret < 0)
> + return;
> +
> + /* Wait for pending CBC completion if download nv returns 2 */
> + if (ret == 2) {

Perhaps have a #define for the value is warranted

> + ret = wait_for_completion_timeout(>cbc, 
> WCNSS_REQUEST_TIMEOUT);
> + if (!ret)
> + dev_err(wcnss->dev, "expected cbc completion\n");
> + }
> +
> + of_platform_populate(wcnss->dev->of_node, NULL, NULL, wcnss->dev);
>  }

Re: [PATCH 4/5] soc: qcom: wcnss_ctrl: Make wcnss_ctrl parent the other components

2016-03-31 Thread Andy Gross 
On Mon, Mar 28, 2016 at 09:35:25PM -0700, Bjorn Andersson wrote:
> We need the signal from wcnss_ctrl indicating that the firmware is up
> and running before we can communicate with the other components of the
> chip. So make these other components children of the wcnss_ctrl device,
> so they can be probed in order.
> 
> The process seems to take between 1/2-5 seconds, so this is done in a
> worker, instead of holding up the probe.
> 
> Also adding the wait for a cbc completion if the firmware indicates this is
> needed - like on 8016.

Can you define what a CBC is?




>  /**
>   * wcnss_download_nv() - send nv binary to WCNSS
> - * @work:work struct to acquire wcnss context
> + * @wcnss:   wcnss_ctrl state handle
> + *
> + * Returns 1 on success or 2 to indicate upcoming cbc completion. Negative

maybe a nit, but define the return values

> + * errno on failure.
>   */
> -static void wcnss_download_nv(struct work_struct *work)
> +static int wcnss_download_nv(struct wcnss_ctrl *wcnss)
>  {
> - struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> download_nv_work);
>   struct wcnss_download_nv_req *req;
>   const struct firmware *fw;
>   const void *data;
> @@ -178,7 +207,7 @@ static void wcnss_download_nv(struct work_struct *work)
>  
>   req = kzalloc(sizeof(*req) + NV_FRAGMENT_SIZE, GFP_KERNEL);
>   if (!req)
> - return;
> + return -ENOMEM;
>  
>   ret = request_firmware(, NVBIN_FILE, wcnss->dev);
>   if (ret) {



> +static void wcnss_async_probe(struct work_struct *work)
> +{
> + struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
> probe_work);
> + int ret;
> +
> + ret = wcnss_request_version(wcnss);
> + if (ret < 0)
> + return;
> +
> + ret = wcnss_download_nv(wcnss);
> + if (ret < 0)
> + return;
> +
> + /* Wait for pending CBC completion if download nv returns 2 */
> + if (ret == 2) {

Perhaps have a #define for the value is warranted

> + ret = wait_for_completion_timeout(>cbc, 
> WCNSS_REQUEST_TIMEOUT);
> + if (!ret)
> + dev_err(wcnss->dev, "expected cbc completion\n");
> + }
> +
> + of_platform_populate(wcnss->dev->of_node, NULL, NULL, wcnss->dev);
>  }

[PATCH 4/5] soc: qcom: wcnss_ctrl: Make wcnss_ctrl parent the other components

2016-03-28 Thread Bjorn Andersson 
We need the signal from wcnss_ctrl indicating that the firmware is up
and running before we can communicate with the other components of the
chip. So make these other components children of the wcnss_ctrl device,
so they can be probed in order.

The process seems to take between 1/2-5 seconds, so this is done in a
worker, instead of holding up the probe.

Also adding the wait for a cbc completion if the firmware indicates this is
needed - like on 8016.

Signed-off-by: Bjorn Andersson 
---
 drivers/soc/qcom/wcnss_ctrl.c   | 117 ++--
 include/linux/soc/qcom/wcnss_ctrl.h |   8 +++
 2 files changed, 108 insertions(+), 17 deletions(-)
 create mode 100644 include/linux/soc/qcom/wcnss_ctrl.h

diff --git a/drivers/soc/qcom/wcnss_ctrl.c b/drivers/soc/qcom/wcnss_ctrl.c
index c544f3d2c6ee..80bea66889a2 100644
--- a/drivers/soc/qcom/wcnss_ctrl.c
+++ b/drivers/soc/qcom/wcnss_ctrl.c
@@ -1,4 +1,5 @@
 /*
+ * Copyright (c) 2016, Linaro Ltd.
  * Copyright (c) 2015, Sony Mobile Communications Inc.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -14,8 +15,13 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 
 #define WCNSS_REQUEST_TIMEOUT  (5 * HZ)
+#define WCNSS_CBC_TIMEOUT  (10 * HZ)
 
 #define NV_FRAGMENT_SIZE   3072
 #define NVBIN_FILE "wlan/prima/WCNSS_qcom_wlan_nv.bin"
@@ -25,17 +31,19 @@
  * @dev:   device handle
  * @channel:   SMD channel handle
  * @ack:   completion for outstanding requests
+ * @cbc:   completion for cbc complete indication
  * @ack_status:status of the outstanding request
- * @download_nv_work: worker for uploading nv binary
+ * @probe_work: worker for uploading nv binary
  */
 struct wcnss_ctrl {
struct device *dev;
struct qcom_smd_channel *channel;
 
struct completion ack;
+   struct completion cbc;
int ack_status;
 
-   struct work_struct download_nv_work;
+   struct work_struct probe_work;
 };
 
 /* message types */
@@ -48,6 +56,11 @@ enum {
WCNSS_UPLOAD_CAL_RESP,
WCNSS_DOWNLOAD_CAL_REQ,
WCNSS_DOWNLOAD_CAL_RESP,
+   WCNSS_VBAT_LEVEL_IND,
+   WCNSS_BUILD_VERSION_REQ,
+   WCNSS_BUILD_VERSION_RESP,
+   WCNSS_PM_CONFIG_REQ,
+   WCNSS_CBC_COMPLETE_IND,
 };
 
 /**
@@ -128,7 +141,7 @@ static int wcnss_ctrl_smd_callback(struct qcom_smd_channel 
*channel,
 version->major, version->minor,
 version->version, version->revision);
 
-   schedule_work(>download_nv_work);
+   complete(>ack);
break;
case WCNSS_DOWNLOAD_NV_RESP:
if (count != sizeof(*nvresp)) {
@@ -141,6 +154,10 @@ static int wcnss_ctrl_smd_callback(struct qcom_smd_channel 
*channel,
wcnss->ack_status = nvresp->status;
complete(>ack);
break;
+   case WCNSS_CBC_COMPLETE_IND:
+   dev_dbg(wcnss->dev, "WCNSS booted\n");
+   complete(>cbc);
+   break;
default:
dev_info(wcnss->dev, "unknown message type %d\n", hdr->type);
break;
@@ -156,20 +173,32 @@ static int wcnss_ctrl_smd_callback(struct 
qcom_smd_channel *channel,
 static int wcnss_request_version(struct wcnss_ctrl *wcnss)
 {
struct wcnss_msg_hdr msg;
+   int ret;
 
msg.type = WCNSS_VERSION_REQ;
msg.len = sizeof(msg);
+   ret = qcom_smd_send(wcnss->channel, , sizeof(msg));
+   if (ret < 0)
+   return ret;
+
+   ret = wait_for_completion_timeout(>ack, WCNSS_CBC_TIMEOUT);
+   if (!ret) {
+   dev_err(wcnss->dev, "timeout waiting for version response\n");
+   return -ETIMEDOUT;
+   }
 
-   return qcom_smd_send(wcnss->channel, , sizeof(msg));
+   return 0;
 }
 
 /**
  * wcnss_download_nv() - send nv binary to WCNSS
- * @work:  work struct to acquire wcnss context
+ * @wcnss: wcnss_ctrl state handle
+ *
+ * Returns 1 on success or 2 to indicate upcoming cbc completion. Negative
+ * errno on failure.
  */
-static void wcnss_download_nv(struct work_struct *work)
+static int wcnss_download_nv(struct wcnss_ctrl *wcnss)
 {
-   struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
download_nv_work);
struct wcnss_download_nv_req *req;
const struct firmware *fw;
const void *data;
@@ -178,7 +207,7 @@ static void wcnss_download_nv(struct work_struct *work)
 
req = kzalloc(sizeof(*req) + NV_FRAGMENT_SIZE, GFP_KERNEL);
if (!req)
-   return;
+   return -ENOMEM;
 
ret = request_firmware(, NVBIN_FILE, wcnss->dev);
if (ret) {
@@ -220,16 +249,56 @@ static void wcnss_download_nv(struct work_struct *work)
} while (left > 0);
 
ret = wait_for_completion_timeout(>ack, WCNSS_REQUEST_TIMEOUT);
-   if (!ret)
+   if (!ret) {

[PATCH 4/5] soc: qcom: wcnss_ctrl: Make wcnss_ctrl parent the other components

2016-03-28 Thread Bjorn Andersson 
We need the signal from wcnss_ctrl indicating that the firmware is up
and running before we can communicate with the other components of the
chip. So make these other components children of the wcnss_ctrl device,
so they can be probed in order.

The process seems to take between 1/2-5 seconds, so this is done in a
worker, instead of holding up the probe.

Also adding the wait for a cbc completion if the firmware indicates this is
needed - like on 8016.

Signed-off-by: Bjorn Andersson 
---
 drivers/soc/qcom/wcnss_ctrl.c   | 117 ++--
 include/linux/soc/qcom/wcnss_ctrl.h |   8 +++
 2 files changed, 108 insertions(+), 17 deletions(-)
 create mode 100644 include/linux/soc/qcom/wcnss_ctrl.h

diff --git a/drivers/soc/qcom/wcnss_ctrl.c b/drivers/soc/qcom/wcnss_ctrl.c
index c544f3d2c6ee..80bea66889a2 100644
--- a/drivers/soc/qcom/wcnss_ctrl.c
+++ b/drivers/soc/qcom/wcnss_ctrl.c
@@ -1,4 +1,5 @@
 /*
+ * Copyright (c) 2016, Linaro Ltd.
  * Copyright (c) 2015, Sony Mobile Communications Inc.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -14,8 +15,13 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 
 #define WCNSS_REQUEST_TIMEOUT  (5 * HZ)
+#define WCNSS_CBC_TIMEOUT  (10 * HZ)
 
 #define NV_FRAGMENT_SIZE   3072
 #define NVBIN_FILE "wlan/prima/WCNSS_qcom_wlan_nv.bin"
@@ -25,17 +31,19 @@
  * @dev:   device handle
  * @channel:   SMD channel handle
  * @ack:   completion for outstanding requests
+ * @cbc:   completion for cbc complete indication
  * @ack_status:status of the outstanding request
- * @download_nv_work: worker for uploading nv binary
+ * @probe_work: worker for uploading nv binary
  */
 struct wcnss_ctrl {
struct device *dev;
struct qcom_smd_channel *channel;
 
struct completion ack;
+   struct completion cbc;
int ack_status;
 
-   struct work_struct download_nv_work;
+   struct work_struct probe_work;
 };
 
 /* message types */
@@ -48,6 +56,11 @@ enum {
WCNSS_UPLOAD_CAL_RESP,
WCNSS_DOWNLOAD_CAL_REQ,
WCNSS_DOWNLOAD_CAL_RESP,
+   WCNSS_VBAT_LEVEL_IND,
+   WCNSS_BUILD_VERSION_REQ,
+   WCNSS_BUILD_VERSION_RESP,
+   WCNSS_PM_CONFIG_REQ,
+   WCNSS_CBC_COMPLETE_IND,
 };
 
 /**
@@ -128,7 +141,7 @@ static int wcnss_ctrl_smd_callback(struct qcom_smd_channel 
*channel,
 version->major, version->minor,
 version->version, version->revision);
 
-   schedule_work(>download_nv_work);
+   complete(>ack);
break;
case WCNSS_DOWNLOAD_NV_RESP:
if (count != sizeof(*nvresp)) {
@@ -141,6 +154,10 @@ static int wcnss_ctrl_smd_callback(struct qcom_smd_channel 
*channel,
wcnss->ack_status = nvresp->status;
complete(>ack);
break;
+   case WCNSS_CBC_COMPLETE_IND:
+   dev_dbg(wcnss->dev, "WCNSS booted\n");
+   complete(>cbc);
+   break;
default:
dev_info(wcnss->dev, "unknown message type %d\n", hdr->type);
break;
@@ -156,20 +173,32 @@ static int wcnss_ctrl_smd_callback(struct 
qcom_smd_channel *channel,
 static int wcnss_request_version(struct wcnss_ctrl *wcnss)
 {
struct wcnss_msg_hdr msg;
+   int ret;
 
msg.type = WCNSS_VERSION_REQ;
msg.len = sizeof(msg);
+   ret = qcom_smd_send(wcnss->channel, , sizeof(msg));
+   if (ret < 0)
+   return ret;
+
+   ret = wait_for_completion_timeout(>ack, WCNSS_CBC_TIMEOUT);
+   if (!ret) {
+   dev_err(wcnss->dev, "timeout waiting for version response\n");
+   return -ETIMEDOUT;
+   }
 
-   return qcom_smd_send(wcnss->channel, , sizeof(msg));
+   return 0;
 }
 
 /**
  * wcnss_download_nv() - send nv binary to WCNSS
- * @work:  work struct to acquire wcnss context
+ * @wcnss: wcnss_ctrl state handle
+ *
+ * Returns 1 on success or 2 to indicate upcoming cbc completion. Negative
+ * errno on failure.
  */
-static void wcnss_download_nv(struct work_struct *work)
+static int wcnss_download_nv(struct wcnss_ctrl *wcnss)
 {
-   struct wcnss_ctrl *wcnss = container_of(work, struct wcnss_ctrl, 
download_nv_work);
struct wcnss_download_nv_req *req;
const struct firmware *fw;
const void *data;
@@ -178,7 +207,7 @@ static void wcnss_download_nv(struct work_struct *work)
 
req = kzalloc(sizeof(*req) + NV_FRAGMENT_SIZE, GFP_KERNEL);
if (!req)
-   return;
+   return -ENOMEM;
 
ret = request_firmware(, NVBIN_FILE, wcnss->dev);
if (ret) {
@@ -220,16 +249,56 @@ static void wcnss_download_nv(struct work_struct *work)
} while (left > 0);
 
ret = wait_for_completion_timeout(>ack, WCNSS_REQUEST_TIMEOUT);
-   if (!ret)
+   if (!ret) {