[PATCH v6 03/10] ath10k: htc: move htc ctrl ep connect to htc_init

[Erik Stromdahl]

On 2017-03-16 02:06, Ryan Hsu wrote:
> On 03/15/2017 08:45 AM, Kalle Valo wrote:
>
>> From: Erik Stromdahl <erik.stromdahl at gmail.com>
>>
>> This patch moves the HTC ctrl service connect from
>> htc_wait_target to htc_init.
>>
>> This is done in order to make sure the htc ctrl service
>> is setup properly before hif_start is called.
>>
>> The reason for this is that we want the HTC ctrl service
>> callback to be initialized before the target sends the
>> HTC ready message.
>>
>> The ready message will always be transmitted on endpoint 0
>> (which is always assigned to the HTC control service) so it
>> makes more sense if HTC control has been connected before the
>> ready message is received.
>>
>> Since the service to pipe mapping is done as a part of
>> the service connect, the get_default_pipe call is redundant
>> and was removed.
>>
>> Signed-off-by: Erik Stromdahl <erik.stromdahl at gmail.com>
>> Signed-off-by: Kalle Valo <kvalo at qca.qualcomm.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/htc.c |   39 ++++++++++++++-------------------
>>  1 file changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
>> index fd5be4484984..92d5ac45327a 100644
>> --- a/drivers/net/wireless/ath/ath10k/htc.c
>> +++ b/drivers/net/wireless/ath/ath10k/htc.c
>> @@ -586,8 +586,6 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
>>  	struct ath10k *ar = htc->ar;
>>  	int i, status = 0;
>>  	unsigned long time_left;
>> -	struct ath10k_htc_svc_conn_req conn_req;
>> -	struct ath10k_htc_svc_conn_resp conn_resp;
>>  	struct ath10k_htc_msg *msg;
>>  	u16 message_id;
>>  	u16 credit_count;
>> @@ -650,22 +648,6 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
>>  		return -ECOMM;
>>  	}
>>
>> -	/* setup our pseudo HTC control endpoint connection */
>> -	memset(&conn_req, 0, sizeof(conn_req));
>> -	memset(&conn_resp, 0, sizeof(conn_resp));
>> -	conn_req.ep_ops.ep_tx_complete = ath10k_htc_control_tx_complete;
>> -	conn_req.ep_ops.ep_rx_complete = ath10k_htc_control_rx_complete;
>> -	conn_req.max_send_queue_depth = ATH10K_NUM_CONTROL_TX_BUFFERS;
>> -	conn_req.service_id = ATH10K_HTC_SVC_ID_RSVD_CTRL;
>> -
>> -	/* connect fake service */
>> -	status = ath10k_htc_connect_service(htc, &conn_req, &conn_resp);
>> -	if (status) {
>> -		ath10k_err(ar, "could not connect to htc service (%d)\n",
>> -			   status);
>> -		return status;
>> -	}
>> -
>>  	return 0;
>>  }
>>
>> @@ -875,8 +857,10 @@ int ath10k_htc_start(struct ath10k_htc *htc)
>>  /* registered target arrival callback from the HIF layer */
>>  int ath10k_htc_init(struct ath10k *ar)
>>  {
>> -	struct ath10k_htc_ep *ep = NULL;
>> +	int status;
>>  	struct ath10k_htc *htc = &ar->htc;
>> +	struct ath10k_htc_svc_conn_req conn_req;
>> +	struct ath10k_htc_svc_conn_resp conn_resp;
>>
>>  	spin_lock_init(&htc->tx_lock);
>>
>> @@ -884,10 +868,21 @@ int ath10k_htc_init(struct ath10k *ar)
>>
>>  	htc->ar = ar;
>>
>> -	/* Get HIF default pipe for HTC message exchange */
>> -	ep = &htc->endpoint[ATH10K_HTC_EP_0];
>> +	/* setup our pseudo HTC control endpoint connection */
>> +	memset(&conn_req, 0, sizeof(conn_req));
>> +	memset(&conn_resp, 0, sizeof(conn_resp));
>> +	conn_req.ep_ops.ep_tx_complete = ath10k_htc_control_tx_complete;
>> +	conn_req.ep_ops.ep_rx_complete = ath10k_htc_control_rx_complete;
>> +	conn_req.max_send_queue_depth = ATH10K_NUM_CONTROL_TX_BUFFERS;
>> +	conn_req.service_id = ATH10K_HTC_SVC_ID_RSVD_CTRL;
>>
>> -	ath10k_hif_get_default_pipe(ar, &ep->ul_pipe_id, &ep->dl_pipe_id);
>> +	/* connect fake service */
>> +	status = ath10k_htc_connect_service(htc, &conn_req, &conn_resp);
>> +	if (status) {
>> +		ath10k_err(ar, "could not connect to htc service (%d)\n",
>> +			   status);
>> +		return status;
>> +	}
>>
>
> I haven't tracked down why, but just curious.
> ath10K-htc_connect_service() will use ath10k_htc_send() and ath10k_hif_tx interface to send data down.
> I am not sure if this is proper to just move it before ath10k_hif_init() is called, will have to check further...
>
The service connect of service 1 (ATH10K_HTC_SVC_ID_RSVD_CTRL) will not result in any bus
traffic (no ath10k_htc_send will be invoked).

ath10k_htc_connect_service has a special case for this service that assigns it to endpoint 0
always.

I initially had a comment about this in the commit log, but it "got lost" during some rebase...

>>  	init_completion(&htc->ctl_resp);
>>
>
> ath10k_htc_connect_service() will expect to use htc->ctrl_resp to synchronize.
> Though there is a reinit_completion() call inside that, but logically, you should still do the init_completion() before using it.
>
>
> _______________________________________________
> ath10k mailing list
> ath10k at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k
>