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.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
+ 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?
+
+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?
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/?
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?
+} __packed __aligned(4);
rest snipped
