> -----Original Message-----
> From: Jeff Johnson <quic_jjohnson@xxxxxxxxxxx>
> Sent: Friday, August 19, 2022 1:03 AM
> To: Kalle Valo <kvalo@xxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Cc: ath12k@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 27/50] wifi: ath12k: add htc.c
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On 8/12/2022 9:09 AM, Kalle Valo wrote:
> > From: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> >
> > (Patches split into one patch per file for easier review, but the
> > final commit will be one big patch. See the cover letter for more
> > info.)
> >
> > Signed-off-by: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> > ---
> > drivers/net/wireless/ath/ath12k/htc.c | 789
> ++++++++++++++++++++++++++++++++++
> > 1 file changed, 789 insertions(+)
> >
> > diff --git a/drivers/net/wireless/ath/ath12k/htc.c
> > b/drivers/net/wireless/ath/ath12k/htc.c
>
> snip
>
> > +static struct sk_buff *ath12k_htc_build_tx_ctrl_skb(void *ab)
>
> why void * instead of struct ath12k_base *?
Sure will address this comment in the next version of the patch
>
> > +{
> > + struct sk_buff *skb;
> > + struct ath12k_skb_cb *skb_cb;
> > +
> > + skb = dev_alloc_skb(ATH12K_HTC_CONTROL_BUFFER_SIZE);
> > + if (!skb)
> > + return NULL;
> > +
> > + skb_reserve(skb, sizeof(struct ath12k_htc_hdr));
> > + WARN_ON_ONCE(!IS_ALIGNED((unsigned long)skb->data, 4));
> > +
> > + skb_cb = ATH12K_SKB_CB(skb);
> > + memset(skb_cb, 0, sizeof(*skb_cb));
> > +
> > + ath12k_dbg(ab, ATH12K_DBG_HTC, "%s: skb %pK\n", __func__, skb);
>
> does this log really serve any useful purpose?
Sure will address this comment in the next version of the patch
>
> > + return skb;
> > +}
> > +
>
> snip
>
> > +static u8 ath12k_htc_get_credit_allocation(struct ath12k_htc *htc,
> > + u16 service_id) {
> > + u8 i, allocation = 0;
> > +
> > + for (i = 0; i < ATH12K_HTC_MAX_SERVICE_ALLOC_ENTRIES; i++) {
> > + if (htc->service_alloc_table[i].service_id == service_id) {
> > + allocation =
> > +
> > + htc->service_alloc_table[i].credit_allocation;
>
> should you break from the loop here, or can there be more/bigger
> allocations for the same service id?
>
Sure will address this comment in the next version of the patch
> > + }
> > + }
> > +
> > + return allocation;
> > +}
> > +
> > +static int ath12k_htc_setup_target_buffer_assignments(struct
> > +ath12k_htc *htc) {
> > + struct ath12k_htc_svc_tx_credits *serv_entry;
> > + u32 svc_id[] = {
>
> static const?
>
> > + ATH12K_HTC_SVC_ID_WMI_CONTROL,
> > + ATH12K_HTC_SVC_ID_WMI_CONTROL_MAC1,
> > + ATH12K_HTC_SVC_ID_WMI_CONTROL_MAC2,
> > + };
> > + int i, credits;
> > +
> > + credits = htc->total_transmit_credits;
> > + serv_entry = htc->service_alloc_table;
> > +
> > + if ((htc->wmi_ep_count == 0) ||
> > + (htc->wmi_ep_count > ARRAY_SIZE(svc_id)))
> > + return -EINVAL;
> > +
> > + /* Divide credits among number of endpoints for WMI */
> > + credits = credits / htc->wmi_ep_count;
> > + for (i = 0; i < htc->wmi_ep_count; i++) {
> > + serv_entry[i].service_id = svc_id[i];
> > + serv_entry[i].credit_allocation = credits;
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> snip
>
> > +int ath12k_htc_start(struct ath12k_htc *htc) {
> > + struct sk_buff *skb;
> > + int status = 0;
>
> nit: initializer always overwritten by ath12k_htc_send()?
>
Sure will address this comment in the next version of the patch
> > + struct ath12k_base *ab = htc->ab;
> > + struct ath12k_htc_setup_complete_extended *msg;
> > +
> > + skb = ath12k_htc_build_tx_ctrl_skb(htc->ab);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + skb_put(skb, sizeof(*msg));
> > + memset(skb->data, 0, skb->len);
> > +
> > + msg = (struct ath12k_htc_setup_complete_extended *)skb->data;
> > + msg->msg_id =
> le32_encode_bits(ATH12K_HTC_MSG_SETUP_COMPLETE_EX_ID,
> > + HTC_MSG_MESSAGEID);
> > +
> > + ath12k_dbg(ab, ATH12K_DBG_HTC, "HTC is using TX credit flow
> > + control\n");
> > +
> > + status = ath12k_htc_send(htc, ATH12K_HTC_EP_0, skb);
> > + if (status) {
> > + kfree_skb(skb);
> > + return status;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int ath12k_htc_init(struct ath12k_base *ab) {
> > + struct ath12k_htc *htc = &ab->htc;
> > + struct ath12k_htc_svc_conn_req conn_req;
> > + struct ath12k_htc_svc_conn_resp conn_resp;
> > + int ret;
> > +
> > + spin_lock_init(&htc->tx_lock);
> > +
> > + ath12k_htc_reset_endpoint_states(htc);
> > +
> > + htc->ab = ab;
> > +
> > + switch (ab->wmi_ab.preferred_hw_mode) {
> > + case WMI_HOST_HW_MODE_SINGLE:
> > + htc->wmi_ep_count = 1;
> > + break;
> > + case WMI_HOST_HW_MODE_DBS:
> > + case WMI_HOST_HW_MODE_DBS_OR_SBS:
> > + htc->wmi_ep_count = 2;
> > + break;
> > + case WMI_HOST_HW_MODE_DBS_SBS:
> > + htc->wmi_ep_count = 3;
> > + break;
> > + default:
> > + htc->wmi_ep_count = ab->hw_params->max_radios;
> > + break;
> > + }
> > +
> > + /* setup our pseudo HTC control endpoint connection */
> > + memset(&conn_req, 0, sizeof(conn_req));
> > + memset(&conn_resp, 0, sizeof(conn_resp));
>
> is this better than having = {} initializers?
>
> > + conn_req.ep_ops.ep_tx_complete =
> ath12k_htc_control_tx_complete;
> > + conn_req.ep_ops.ep_rx_complete =
> ath12k_htc_control_rx_complete;
> > + conn_req.max_send_queue_depth =
> ATH12K_NUM_CONTROL_TX_BUFFERS;
> > + conn_req.service_id = ATH12K_HTC_SVC_ID_RSVD_CTRL;
> > +
> > + /* connect fake service */
> > + ret = ath12k_htc_connect_service(htc, &conn_req, &conn_resp);
> > + if (ret) {
> > + ath12k_err(ab, "could not connect to htc service (%d)\n", ret);
> > + return ret;
> > + }
> > +
> > + init_completion(&htc->ctl_resp);
> > +
> > + return 0;
> > +}
> >
Thanks
Karthikeyan
