[PATCH 28/50] wifi: ath12k: add htc.h

[Karthikeyan Periyasamy (QUIC)]

> -----Original Message-----
> From: Jeff Johnson <quic_jjohnson at quicinc.com>
> Sent: Friday, August 19, 2022 2:41 AM
> To: Kalle Valo <kvalo at kernel.org>; linux-wireless at vger.kernel.org
> Cc: ath12k at lists.infradead.org
> Subject: Re: [PATCH 28/50] wifi: ath12k: add htc.h
> 
> 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 at quicinc.com>
> >
> > (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 at quicinc.com>
> > ---
> >   drivers/net/wireless/ath/ath12k/htc.h | 311
> ++++++++++++++++++++++++++++++++++
> >   1 file changed, 311 insertions(+)
> >
> > diff --git a/drivers/net/wireless/ath/ath12k/htc.h
> > b/drivers/net/wireless/ath/ath12k/htc.h
> 
> snip
> 
> > +#define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK
> GENMASK(1, 0)
> > +#define ATH12K_HTC_CONN_FLAGS_RECV_ALLOC GENMASK(15, 8)
> > +
> > +enum ath12k_htc_conn_flags {
> > +     ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_FOURTH    =
> 0x0,
> > +     ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_HALF      = 0x1,
> > +     ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_THREE_FOURTHS =
> 0x2,
> > +     ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_UNITY         = 0x3,
> 
> it seems strange to call a 2-bit field a flag

Sure will address this comment in the next version of the patch

> 
> > +     ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE    = 1 << 2,
> > +     ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL = 1 << 3 };
> 
> and it seems strange to have an enum which incorporates the values of the
> ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK along with
> definitions of other mask flags.
> 
> perhaps these would be more logically defined as macros just after
> ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK:
> #define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK
> GENMASK(1, 0) #define
> ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE BIT(2) #define
> ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL BIT(3)
> 
> and then rename ath12k_htc_conn_flags to be more specific that it just has
> threshold level values?
> 

Sure will address this comment in the next version of the patch

> > +
> > +enum ath12k_htc_conn_svc_status {
> > +     ATH12K_HTC_CONN_SVC_STATUS_SUCCESS      = 0,
> > +     ATH12K_HTC_CONN_SVC_STATUS_NOT_FOUND    = 1,
> > +     ATH12K_HTC_CONN_SVC_STATUS_FAILED       = 2,
> > +     ATH12K_HTC_CONN_SVC_STATUS_NO_RESOURCES = 3,
> > +     ATH12K_HTC_CONN_SVC_STATUS_NO_MORE_EP   = 4
> > +};
> > +
> > +struct ath12k_htc_ready {
> > +     __le32 id_credit_count;
> > +     __le32 size_ep;
> > +} __packed;
> > +
> > +struct ath12k_htc_ready_extended {
> > +     struct ath12k_htc_ready base;
> > +     __le32 ver_bundle;
> > +} __packed;
> > +
> > +struct ath12k_htc_conn_svc {
> > +     __le32 msg_svc_id;
> > +     __le32 flags_len;
> > +} __packed;
> > +
> > +struct ath12k_htc_conn_svc_resp {
> > +     __le32 msg_svc_id;
> > +     __le32 flags_len;
> > +     __le32 svc_meta_pad;
> > +} __packed;
> > +
> > +struct ath12k_htc_setup_complete_extended {
> > +     __le32 msg_id;
> 
> is there a reason this isn't msg_svc_id to be consistent with the other htc
> messages?
> 

Yes, its different.

> or even have every htc msg begin with a struct ath12k_htc_msg hdr?
> 
> perhaps also consider some naming conventions similar to what were
> adopted for WMI?
> 
> > +     __le32 flags;
> > +     __le32 max_msgs_per_bundled_recv; } __packed;
> > +
> > +struct ath12k_htc_msg {
> > +     __le32 msg_svc_id;
> > +     __le32 flags_len;
> > +} __packed __aligned(4);
> > +
> > +enum ath12k_htc_record_id {
> > +     ATH12K_HTC_RECORD_NULL    = 0,
> > +     ATH12K_HTC_RECORD_CREDITS = 1
> > +};
> > +
> > +struct ath12k_htc_record_hdr {
> > +     u8 id; /* @enum ath12k_htc_record_id */
> > +     u8 len;
> > +     u8 pad0;
> > +     u8 pad1;
> > +} __packed;
> > +
> > +struct ath12k_htc_credit_report {
> > +     u8 eid; /* @enum ath12k_htc_ep_id */
> > +     u8 credits;
> > +     u8 pad0;
> > +     u8 pad1;
> > +} __packed;
> > +
> > +struct ath12k_htc_record {
> > +     struct ath12k_htc_record_hdr hdr;
> > +     union {
> > +             struct ath12k_htc_credit_report credit_report[0];
> > +             u8 pauload[0];
> 
> s/pauload/payload/?
> 

Sure will address this comment in the next version of the patch

> s/[0]/[]/ per
> <https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-
> length-and-one-element-arrays>
> 
> > +     };
> > +} __packed __aligned(4);
> > +
> > +/* note: the trailer offset is dynamic depending
> > + * on payload length. this is only a struct layout draft  */ struct
> > +ath12k_htc_frame {
> > +     struct ath12k_htc_hdr hdr;
> > +     union {
> > +             struct ath12k_htc_msg msg;
> > +             u8 payload[0];
> 
> s/[0]/[]/
> 
> > +     };
> > +     struct ath12k_htc_record trailer[0];
> 
> 
> s/[0]/[]/
> 
> but this struct makes no sense. you can't have a variable-length payload that
> isn't at the end of the struct
> 
> if this is just supposed to be some sort of pseudo-documentation then
> perhaps put it in a comment so that the compiler won't see this?
> 
> or perhaps even better draw an ASCII representation?

Sure will address this comment in the next version of the patch

> 
> > +} __packed __aligned(4);
> 
> rest snipped

Thanks
Karthikeyan