| Message ID | 20231030222700.18914-1-quic_rajkbhag@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: ath12k: QCN9274 dualmac bring up | expand |
On 10/30/2023 3:26 PM, Raj Kumar Bhagat wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > When any VDEV is started, MBSSID flags are passed to firmware to > indicate if its a MBSSID/EMA AP vdev. If the interface is not an AP > or if the AP doesn't support MBSSID, the vdev needs to be brought up > as a non MBSSID vdev. Set these flags as a non MBSSID AP by default > which can be updated as and when MBSSID support is added in ath12k. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Sriram R <quic_srirrama@quicinc.com> > Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/mac.c | 5 +++++ > drivers/net/wireless/ath/ath12k/wmi.c | 1 + > drivers/net/wireless/ath/ath12k/wmi.h | 8 ++++++++ > 3 files changed, 14 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c > index fc0d14ea3..594aa18e7 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.c > +++ b/drivers/net/wireless/ath/ath12k/mac.c > @@ -5987,6 +5987,11 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif, > arg.pref_tx_streams = ar->num_tx_chains; > arg.pref_rx_streams = ar->num_rx_chains; > > + /* Fill the MBSSID flags to indicate AP is non MBSSID by default > + * Corresponding flags would be updated with MBSSID support. > + */ > + arg.mbssid_flags = WMI_VDEV_FLAGS_NON_MBSSID_AP; > + > if (arvif->vdev_type == WMI_VDEV_TYPE_AP) { > arg.ssid = arvif->u.ap.ssid; > arg.ssid_len = arvif->u.ap.ssid_len; > diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c > index 0e5bf5ce8..88ec77dee 100644 > --- a/drivers/net/wireless/ath/ath12k/wmi.c > +++ b/drivers/net/wireless/ath/ath12k/wmi.c > @@ -1024,6 +1024,7 @@ int ath12k_wmi_vdev_start(struct ath12k *ar, struct wmi_vdev_start_req_arg *arg, > cmd->regdomain = cpu_to_le32(arg->regdomain); > cmd->he_ops = cpu_to_le32(arg->he_ops); > cmd->punct_bitmap = cpu_to_le32(arg->punct_bitmap); > + cmd->mbssid_flags = cpu_to_le32(arg->mbssid_flags); > > if (!restart) { > if (arg->ssid) { > diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h > index 8e1eda7aa..dfe9eb0cb 100644 > --- a/drivers/net/wireless/ath/ath12k/wmi.h > +++ b/drivers/net/wireless/ath/ath12k/wmi.h > @@ -2269,6 +2269,14 @@ struct ath12k_wmi_hal_reg_capabilities_ext_arg { > u32 high_5ghz_chan; > }; > > +enum { > + WMI_VDEV_FLAGS_NON_MBSSID_AP = BIT(0), only the 1st one is used. do we need the rest at this point? perhaps add the others as they are needed? > + WMI_VDEV_FLAGS_TRANSMIT_AP = BIT(1), > + WMI_VDEV_FLAGS_NON_TRANSMIT_AP = BIT(2), > + WMI_VDEV_FLAGS_EMA_MODE = BIT(3), > + WMI_VDEV_FLAGS_SCAN_MODE_VAP = BIT(4), note that "vap" is a non-standard term that ideally should not be used in ath12k (although a few references are present) > +}; these seem to be added at some random place in the file. considering that these are applicable to the mbssid_flags in struct wmi_vdev_start_request_cmd, I'd suggest: 1) defining the enum just before this struct 2) naming the enum, i.e. wmi_vdev_mbssid_flags 3) using a prefix that matches the name, i.e. WMI_VDEV_MBSSID_FLAGS_NON_MBSSID_AP 4) in struct wmi_vdev_start_request_cmd add a comment to: __le32 mbssid_flags; /* uses enum wmi_vdev_mbssid_flags */ /jeff
On 10/30/2023 3:26 PM, Raj Kumar Bhagat wrote: > From: Karthikeyan Kathirvel <quic_kathirve@quicinc.com> > > Most of the RX descriptors fields are currently not used in the > ath12k driver. Hence add support to selectively subscribe to the > required quad words (64 bits) within msdu_end and mpdu_start of > rx_desc. > > Add compact rx_desc structures and configure the bit mask for Rx TLVs > (msdu_end, mpdu_start, mpdu_end) via registers. With these registers > SW can configure to DMA the partial TLV struct to Rx buffer. > > Each TLV type has its own register to configure the mask value. > The mask value configured in register will indicate if a particular > QWORD has to be written to rx buffer or not i.e., if Nth bit is enabled > in the mask Nth QWORD will be written and it will not be written if the > bit is disabled in mask. While 0th bit indicates whether TLV tag will be > written or not. > > Advantages of Qword subscription of TLVs > - Avoid multiple cache-line misses as the all the required fields > of the TLV are within 128 bytes. > - Memory optimization as TLVs + DATA + SHINFO can fit in 2k buffer > even for 64 bit kernel. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Karthikeyan Kathirvel <quic_kathirve@quicinc.com> > Co-developed-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com> > Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/dp.c | 9 + > drivers/net/wireless/ath/ath12k/dp.h | 13 + > drivers/net/wireless/ath/ath12k/dp_rx.c | 16 +- > drivers/net/wireless/ath/ath12k/dp_tx.c | 20 ++ > drivers/net/wireless/ath/ath12k/hal.c | 352 ++++++++++++++++++++++ > drivers/net/wireless/ath/ath12k/hal.h | 3 + > drivers/net/wireless/ath/ath12k/rx_desc.h | 112 ++++++- > drivers/net/wireless/ath/ath12k/wmi.h | 2 + > 8 files changed, 519 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c > index 80d7ce44d..faeef965e 100644 > --- a/drivers/net/wireless/ath/ath12k/dp.c > +++ b/drivers/net/wireless/ath/ath12k/dp.c > @@ -1001,6 +1001,15 @@ void ath12k_dp_pdev_pre_alloc(struct ath12k_base *ab) > > void ath12k_dp_hal_rx_desc_init(struct ath12k_base *ab) > { > + if (test_bit(WMI_TLV_SERVICE_WMSK_COMPACTION_RX_TLVS, ab->wmi_ab.svc_map) && > + ab->hw_params->hal_ops->rxdma_ring_wmask_rx_mpdu_start && > + ab->hw_params->hal_ops->rxdma_ring_wmask_rx_msdu_end) { > + /* RX TLVS compaction is supported, hence change the hal_rx_ops > + * based on device. > + */ > + if (ab->hal_rx_ops == &hal_rx_qcn9274_ops) > + ab->hal_rx_ops = &hal_rx_qcn9274_compact_ops; I only have one comment on this patch. in order to avoid chipset-specific logic here suggest that there should be an abstraction. several ideas come to mind: 1) have a hal_ops callback to retrieve it 2) have a pointer to the compact ops in the hw_params since we are already using hal_ops to get the masks, suggest that is where we should get the pointer to the compact ops > + } > ab->hal.hal_desc_sz = > ab->hal_rx_ops->rx_desc_get_desc_size(); > }
On 10/31/2023 4:42 AM, Jeff Johnson wrote: > On 10/30/2023 3:26 PM, Raj Kumar Bhagat wrote: >> From: Sriram R <quic_srirrama@quicinc.com> >> >> When any VDEV is started, MBSSID flags are passed to firmware to >> indicate if its a MBSSID/EMA AP vdev. If the interface is not an AP >> or if the AP doesn't support MBSSID, the vdev needs to be brought up >> as a non MBSSID vdev. Set these flags as a non MBSSID AP by default >> which can be updated as and when MBSSID support is added in ath12k. >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 >> >> Signed-off-by: Sriram R <quic_srirrama@quicinc.com> >> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/mac.c | 5 +++++ >> drivers/net/wireless/ath/ath12k/wmi.c | 1 + >> drivers/net/wireless/ath/ath12k/wmi.h | 8 ++++++++ >> 3 files changed, 14 insertions(+) >> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c >> index fc0d14ea3..594aa18e7 100644 >> --- a/drivers/net/wireless/ath/ath12k/mac.c >> +++ b/drivers/net/wireless/ath/ath12k/mac.c >> @@ -5987,6 +5987,11 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif, >> arg.pref_tx_streams = ar->num_tx_chains; >> arg.pref_rx_streams = ar->num_rx_chains; >> + /* Fill the MBSSID flags to indicate AP is non MBSSID by default >> + * Corresponding flags would be updated with MBSSID support. >> + */ >> + arg.mbssid_flags = WMI_VDEV_FLAGS_NON_MBSSID_AP; >> + >> if (arvif->vdev_type == WMI_VDEV_TYPE_AP) { >> arg.ssid = arvif->u.ap.ssid; >> arg.ssid_len = arvif->u.ap.ssid_len; >> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c >> index 0e5bf5ce8..88ec77dee 100644 >> --- a/drivers/net/wireless/ath/ath12k/wmi.c >> +++ b/drivers/net/wireless/ath/ath12k/wmi.c >> @@ -1024,6 +1024,7 @@ int ath12k_wmi_vdev_start(struct ath12k *ar, struct wmi_vdev_start_req_arg *arg, >> cmd->regdomain = cpu_to_le32(arg->regdomain); >> cmd->he_ops = cpu_to_le32(arg->he_ops); >> cmd->punct_bitmap = cpu_to_le32(arg->punct_bitmap); >> + cmd->mbssid_flags = cpu_to_le32(arg->mbssid_flags); >> if (!restart) { >> if (arg->ssid) { >> diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h >> index 8e1eda7aa..dfe9eb0cb 100644 >> --- a/drivers/net/wireless/ath/ath12k/wmi.h >> +++ b/drivers/net/wireless/ath/ath12k/wmi.h >> @@ -2269,6 +2269,14 @@ struct ath12k_wmi_hal_reg_capabilities_ext_arg { >> u32 high_5ghz_chan; >> }; >> +enum { >> + WMI_VDEV_FLAGS_NON_MBSSID_AP = BIT(0), > > only the 1st one is used. do we need the rest at this point? > perhaps add the others as they are needed? Sure, will add only one - WMI_VDEV_FLAGS_NON_MBSSID_AP. > >> + WMI_VDEV_FLAGS_TRANSMIT_AP = BIT(1), >> + WMI_VDEV_FLAGS_NON_TRANSMIT_AP = BIT(2), >> + WMI_VDEV_FLAGS_EMA_MODE = BIT(3), >> + WMI_VDEV_FLAGS_SCAN_MODE_VAP = BIT(4), > > note that "vap" is a non-standard term that ideally should not be used in ath12k (although a few references are present) > >> +}; > > these seem to be added at some random place in the file. considering that these are applicable to the mbssid_flags in struct wmi_vdev_start_request_cmd, I'd suggest: > > 1) defining the enum just before this struct > 2) naming the enum, i.e. wmi_vdev_mbssid_flags > 3) using a prefix that matches the name, i.e. WMI_VDEV_MBSSID_FLAGS_NON_MBSSID_AP > 4) in struct wmi_vdev_start_request_cmd add a comment to: > __le32 mbssid_flags; /* uses enum wmi_vdev_mbssid_flags */ > > /jeff Will implement the above suggestions in next version.
On 10/31/2023 6:23 AM, Jeff Johnson wrote: > On 10/30/2023 3:26 PM, Raj Kumar Bhagat wrote: >> From: Karthikeyan Kathirvel <quic_kathirve@quicinc.com> >> >> Most of the RX descriptors fields are currently not used in the >> ath12k driver. Hence add support to selectively subscribe to the >> required quad words (64 bits) within msdu_end and mpdu_start of >> rx_desc. >> >> Add compact rx_desc structures and configure the bit mask for Rx TLVs >> (msdu_end, mpdu_start, mpdu_end) via registers. With these registers >> SW can configure to DMA the partial TLV struct to Rx buffer. >> >> Each TLV type has its own register to configure the mask value. >> The mask value configured in register will indicate if a particular >> QWORD has to be written to rx buffer or not i.e., if Nth bit is enabled >> in the mask Nth QWORD will be written and it will not be written if the >> bit is disabled in mask. While 0th bit indicates whether TLV tag will be >> written or not. >> >> Advantages of Qword subscription of TLVs >> - Avoid multiple cache-line misses as the all the required fields >> of the TLV are within 128 bytes. >> - Memory optimization as TLVs + DATA + SHINFO can fit in 2k buffer >> even for 64 bit kernel. >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 >> >> Signed-off-by: Karthikeyan Kathirvel <quic_kathirve@quicinc.com> >> Co-developed-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com> >> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/dp.c | 9 + >> drivers/net/wireless/ath/ath12k/dp.h | 13 + >> drivers/net/wireless/ath/ath12k/dp_rx.c | 16 +- >> drivers/net/wireless/ath/ath12k/dp_tx.c | 20 ++ >> drivers/net/wireless/ath/ath12k/hal.c | 352 ++++++++++++++++++++++ >> drivers/net/wireless/ath/ath12k/hal.h | 3 + >> drivers/net/wireless/ath/ath12k/rx_desc.h | 112 ++++++- >> drivers/net/wireless/ath/ath12k/wmi.h | 2 + >> 8 files changed, 519 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c >> index 80d7ce44d..faeef965e 100644 >> --- a/drivers/net/wireless/ath/ath12k/dp.c >> +++ b/drivers/net/wireless/ath/ath12k/dp.c >> @@ -1001,6 +1001,15 @@ void ath12k_dp_pdev_pre_alloc(struct ath12k_base *ab) >> void ath12k_dp_hal_rx_desc_init(struct ath12k_base *ab) >> { >> + if (test_bit(WMI_TLV_SERVICE_WMSK_COMPACTION_RX_TLVS, ab->wmi_ab.svc_map) && >> + ab->hw_params->hal_ops->rxdma_ring_wmask_rx_mpdu_start && >> + ab->hw_params->hal_ops->rxdma_ring_wmask_rx_msdu_end) { >> + /* RX TLVS compaction is supported, hence change the hal_rx_ops >> + * based on device. >> + */ >> + if (ab->hal_rx_ops == &hal_rx_qcn9274_ops) >> + ab->hal_rx_ops = &hal_rx_qcn9274_compact_ops; > > I only have one comment on this patch. > > in order to avoid chipset-specific logic here suggest that there should be an abstraction. > > several ideas come to mind: > 1) have a hal_ops callback to retrieve it > 2) have a pointer to the compact ops in the hw_params > > since we are already using hal_ops to get the masks, suggest that is where we should get the pointer to the compact ops > Thanks Jeff for the suggestion, will implement hal_ops to retrieve the corresponding compact ops. >> + } >> ab->hal.hal_desc_sz = >> ab->hal_rx_ops->rx_desc_get_desc_size(); >> } >