| Message ID | 20241210074159.2637933-1-quic_rajkbhag@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: ath12k: add Ath12k AHB driver support for IPQ5332 | expand |
On 10/12/2024 08:41, Raj Kumar Bhagat wrote: > + > + time_left = wait_for_completion_timeout(&ab_ahb->userpd_ready, > + ATH12K_USERPD_READY_TIMEOUT); > + if (!time_left) { > + ath12k_err(ab, "UserPD ready wait timed out\n"); > + ret = -ETIMEDOUT; > + goto err_fw2; > + } > + > + qcom_smem_state_update_bits(ab_ahb->spawn_state, BIT(ab_ahb->spawn_bit), 0); > + > + ath12k_info(ab, "UserPD%d is now UP\n", ab_ahb->userpd_id); Don't bug users with success messages. Your driver is supposed to be silent on success. > + > +err_fw2: > + release_firmware(fw2); > +err_fw: > + release_firmware(fw); > + return ret; > +} > + > static void ath12k_ahb_init_qmi_ce_config(struct ath12k_base *ab) > { > struct ath12k_qmi_ce_cfg *cfg = &ab->qmi.ce_cfg; > @@ -557,6 +694,7 @@ static const struct ath12k_hif_ops ath12k_ahb_hif_ops_ipq5332 = { > .irq_enable = ath12k_ahb_ext_irq_enable, > .irq_disable = ath12k_ahb_ext_irq_disable, > .map_service_to_pipe = ath12k_ahb_map_service_to_pipe, > + .power_up = ath12k_ahb_power_up, > }; > > static irqreturn_t ath12k_userpd_irq_handler(int irq, void *data) > diff --git a/drivers/net/wireless/ath/ath12k/ahb.h b/drivers/net/wireless/ath/ath12k/ahb.h > index 0999e2bbe970..0dbbbfd45eab 100644 > --- a/drivers/net/wireless/ath/ath12k/ahb.h > +++ b/drivers/net/wireless/ath/ath12k/ahb.h > @@ -19,7 +19,15 @@ > #define ATH12K_PCI_IRQ_CE0_OFFSET 3 > #define ATH12K_ROOTPD_READY_TIMEOUT (5 * HZ) > #define ATH12K_RPROC_AFTER_POWERUP QCOM_SSR_AFTER_POWERUP > - Why? You just added this line in previous commits. > +#define ATH12K_AHB_FW_PREFIX "q6_fw" > +#define ATH12K_AHB_FW_SUFFIX ".mdt" Best regards, Krzysztof
On 10/12/2024 16:08, Krzysztof Kozlowski wrote: > On 10/12/2024 08:41, Raj Kumar Bhagat wrote: >> Currently, Ath12k driver only supports WiFi devices that are based on >> PCI bus. New Ath12k device IPQ5332 is based on AHB bus. Hence, add >> Ath12k AHB support for IPQ5332. >> >> IPQ5332 is IEEE802.11be 2 GHz 2x2 Wifi device. To bring-up IPQ5332 >> device: >> - Add hardware parameters for IPQ5332. >> - CE register address space in IPQ5332 is separate from WCSS register >> space. Hence, add logic to remap CE register address. >> - Add support for fixed QMI firmware memory for IPQ5332. >> - Support userPD handling for WCSS secure PIL driver to enable ath12k >> AHB support. >> >> v4: >> - Missed to include some review list in v3. Hence sending v4 with >> all review list as per - scripts/get_maintainers.pl >> > The amount of undocumented ABI you add here, points to the problem that > either your drivers don't work or your drivers would never work with > upstream. Why? Because either you would have wrong DTS or drivers not > matching DTS, thus not working. > > Please point us to your upstream DTS implementing (and working 100%) > this ABI, so we can review that you do not sneak more broken or > undocumented things. I will NAK also future submissions without above, > because I believe you usptream something which will not work. I dug a bit and I found your earlier v2: https://lore.kernel.org/all/20241015182637.955753-3-quic_rajkbhag@quicinc.com/ which confirms: 1. DTS not following coding style, so not possible to accept 2. Driver relying on that exact DTS, so not really working. Please post in separate series updated DTS, after fixing all the issues pointed out by DTS coding style. Best regards, Krzysztof
On 12/10/2024 8:13 PM, Krzysztof Kozlowski wrote: > On 10/12/2024 08:41, Raj Kumar Bhagat wrote: >> + case CALDB_MEM_REGION_TYPE: >> + /* Cold boot calibration is not enabled in Ath12k. Hence, >> + * assign paddr = 0. >> + * Once cold boot calibration is enabled add support to >> + * assign reserved memory from DT. >> + */ >> + ab->qmi.target_mem[idx].paddr = 0; >> + ab->qmi.target_mem[idx].v.ioaddr = NULL; >> + ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size; >> + ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type; >> + idx++; >> + break; >> + case M3_DUMP_REGION_TYPE: >> + dev_node = of_find_node_by_name(NULL, "m3_dump"); > > NAK > > That's neither correct name nor documented in the bindings. You created > now undocumented ABI. Even with incorrect name. :/ > Most of the Device Tree related concern in this series are from the undocumented ABIs and wrong naming (use of '_' instead of '-'): "m3_dump" and "mlo_global_mem_0". To address the undocumented ABIs, "memory-region" and "memory-region-names" should be used to reference all the reserved memory required. This should include "m3_dump" and "mlo_global_mem_0" memory region. If the above approach valid to address undocumented ABIs?
On Tue, Dec 10, 2024 at 10:30:54PM +0530, Raj Kumar Bhagat wrote: > On 12/10/2024 8:13 PM, Krzysztof Kozlowski wrote: > > On 10/12/2024 08:41, Raj Kumar Bhagat wrote: > >> + case CALDB_MEM_REGION_TYPE: > >> + /* Cold boot calibration is not enabled in Ath12k. Hence, > >> + * assign paddr = 0. > >> + * Once cold boot calibration is enabled add support to > >> + * assign reserved memory from DT. > >> + */ > >> + ab->qmi.target_mem[idx].paddr = 0; > >> + ab->qmi.target_mem[idx].v.ioaddr = NULL; > >> + ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size; > >> + ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type; > >> + idx++; > >> + break; > >> + case M3_DUMP_REGION_TYPE: > >> + dev_node = of_find_node_by_name(NULL, "m3_dump"); > > > > NAK > > > > That's neither correct name nor documented in the bindings. You created > > now undocumented ABI. Even with incorrect name. :/ > > > > Most of the Device Tree related concern in this series are from the > undocumented ABIs and wrong naming (use of '_' instead of '-'): > "m3_dump" and "mlo_global_mem_0". > > To address the undocumented ABIs, "memory-region" and "memory-region-names" > should be used to reference all the reserved memory required. This should > include "m3_dump" and "mlo_global_mem_0" memory region. You already use them, so this code here and explanation is confusing. > > If the above approach valid to address undocumented ABIs? Dunno. Change all node names, run dtbs_check. Do you see errors? Now run your driver. Does it work correctly? If yes, then ABI seems documented. If not, ABI is not documented. Best regards, Krzysztof