On 1/30/2025 1:27 PM, Krzysztof Kozlowski wrote:
> On 30/01/2025 05:35, Raj Kumar Bhagat wrote:
>> +static int ath12k_ahb_clock_init(struct ath12k_base *ab)
>> +{
>> + struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> + int ret;
>> +
>> + ab_ahb->xo_clk = devm_clk_get(ab->dev, "xo");
>> + if (IS_ERR_OR_NULL(ab_ahb->xo_clk)) {
>
> No, you are not supposed to use IS_ERR_OR_NULL(). That's indication of bug.
>
>> + ret = ab_ahb->xo_clk ? PTR_ERR(ab_ahb->xo_clk) : -ENODEV;
>
> I don't understand this. It's the third time you are reimplementing
> standard code in some odd way, different than all other drivers.
>
> Read the description of this function. Can clk_get return NULL? Of
> course not. This is so overcomplicated for no reason, I wonder if it is
> actually buggy here.
>
My bad, will update in next version.
>
>> + return dev_err_probe(&ab->pdev->dev, ret, "failed to get xo clock\n");
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void ath12k_ahb_clock_deinit(struct ath12k_base *ab)
>> +{
>> + struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> +
>> + devm_clk_put(ab->dev, ab_ahb->xo_clk);
>> + ab_ahb->xo_clk = NULL;
>> +}
>> +
>> +static int ath12k_ahb_clock_enable(struct ath12k_base *ab)
>> +{
>> + struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> + int ret;
>> +
>> + if (IS_ERR_OR_NULL(ab_ahb->xo_clk)) {
>> + ath12k_err(ab, "clock is not initialized\n");
>
> NAK.
>
> Sorry, this code makes no sense. This is some random code. This code
> cannot be executed before probe. If it can: your driver is buggy, so fix
> your driver.
>
> After the probe(), this is never NULL as an error. Either you have here
> valid pointer or you failed the probe.
>
> This driver fails on basics of driver probing.
>
Will remove the above code in next version.
>> + return -EIO;
>> + }
>> +
>> + ret = clk_prepare_enable(ab_ahb->xo_clk);
>> + if (ret) {
>> + ath12k_err(ab, "failed to enable gcc_xo_clk: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void ath12k_ahb_clock_disable(struct ath12k_base *ab)
>> +{
>> + struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> +
>> + clk_disable_unprepare(ab_ahb->xo_clk);
>
> Don't create such wrappers for single clock. Does not help.
>
Sure, will remove the wrapper.
>> +}
>> +
>> +static int ath12k_ahb_resource_init(struct ath12k_base *ab)
>> +{
>> + struct platform_device *pdev = ab->pdev;
>> + struct resource *mem_res;
>> + int ret;
>> +
>> + ab->mem = devm_platform_get_and_ioremap_resource(pdev, 0, &mem_res);
>> + if (IS_ERR(ab->mem)) {
>> + ret = dev_err_probe(&pdev->dev, PTR_ERR(ab->mem), "ioremap error\n");
>> + goto out;
>> + }
>> +
>> + ab->mem_len = resource_size(mem_res);
>> +
>> + if (ab->hw_params->ce_remap) {
>> + const struct ce_remap *ce_remap = ab->hw_params->ce_remap;
>> + /* CE register space is moved out of WCSS and the space is not
>> + * contiguous, hence remapping the CE registers to a new space
>> + * for accessing them.
>> + */
>> + ab->mem_ce = ioremap(ce_remap->base, ce_remap->size);
>> + if (IS_ERR(ab->mem_ce)) {
>> + dev_err(&pdev->dev, "ce ioremap error\n");
>> + ret = -ENOMEM;
>> + goto err_mem_unmap;
>> + }
>> + ab->ce_remap = true;
>> + ab->ce_remap_base_addr = HAL_IPQ5332_CE_WFSS_REG_BASE;
>> + }
>> +
>> + ret = ath12k_ahb_clock_init(ab);
>> + if (ret)
>> + goto err_mem_ce_unmap;
>> +
>> + ret = ath12k_ahb_clock_enable(ab);
>> + if (ret)
>> + goto err_clock_deinit;
>> +
>> + return 0;
>> +
>> +err_clock_deinit:
>> + ath12k_ahb_clock_deinit(ab);
>> +
>> +err_mem_ce_unmap:
>> + if (ab->hw_params->ce_remap)
>> + iounmap(ab->mem_ce);
>> +
>> +err_mem_unmap:
>> + ab->mem_ce = NULL;
>> + devm_iounmap(ab->dev, ab->mem);
>> +
>> +out:
>> + ab->mem = NULL;
>> + return ret;
>> +}
>> +
>> +static void ath12k_ahb_resource_deinit(struct ath12k_base *ab)
>> +{
>> + if (ab->mem)
>> + devm_iounmap(ab->dev, ab->mem);
>> +
>> + if (ab->mem_ce)
>> + iounmap(ab->mem_ce);
>> +
>> + ab->mem = NULL;
>> + ab->mem_ce = NULL;
>> +
>> + ath12k_ahb_clock_disable(ab);
>> + ath12k_ahb_clock_deinit(ab);
>> +}
>> +
>> +static enum ath12k_hw_rev ath12k_ahb_get_hw_rev(struct platform_device *pdev)
>> +{
>> + const struct of_device_id *of_id;
>> +
>> + of_id = of_match_device(ath12k_ahb_of_match, &pdev->dev);
>> + if (!of_id) {
>> + dev_err(&pdev->dev, "Failed to find matching device tree id\n");
>> + return -EINVAL;
>> + }
>> +
>> + return (enum ath12k_hw_rev)of_id->data;
>
> You just open-coded of_device_get_match_data().
>
Thanks, will use of_device_get_match_data() in next version.
>> +}
>> +
>> +static int ath12k_ahb_probe(struct platform_device *pdev)
>> +{
>> + struct ath12k_base *ab;
>> + const struct ath12k_hif_ops *hif_ops;
>> + struct device_node *mem_node;
>> + enum ath12k_hw_rev hw_rev;
>> + u32 addr;
>> + int ret;
>> +
>> + hw_rev = ath12k_ahb_get_hw_rev(pdev);
>> + switch (hw_rev) {
>> + case ATH12K_HW_IPQ5332_HW10:
>> + hif_ops = &ath12k_ahb_hif_ops_ipq5332;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to set 32-bit coherent dma\n");
>> + return ret;
>> + }
>> +
>> + ab = ath12k_core_alloc(&pdev->dev, sizeof(struct ath12k_ahb),
>> + ATH12K_BUS_AHB);
>> + if (!ab) {
>> + dev_err(&pdev->dev, "failed to allocate ath12k base\n");
>
> No, driver never prints allocation errors. You are duplicating existing
> core printk.
>
Thanks, will remove this error print.
>
>> + return -ENOMEM;
>> + }
>> +
>> + ab->hif.ops = hif_ops;
>> + ab->pdev = pdev;
>> + ab->hw_rev = hw_rev;
>> + platform_set_drvdata(pdev, ab);
>> +
>> + /* Set fixed_mem_region to true for platforms that support fixed memory
>> + * reservation from DT. If memory is reserved from DT for FW, ath12k driver
>> + * need not to allocate memory.
>> + */
>> + if (!of_property_read_u32(ab->dev->of_node, "memory-region", &addr)) {
>> + set_bit(ATH12K_FLAG_FIXED_MEM_REGION, &ab->dev_flags);
>> +
>> + /* If the platform supports fixed memory, then it should define/
>> + * reserve MLO global memory in DT to support Multi Link Operation
>> + * (IEEE 802.11be).
>> + * If MLO global memory is not reserved in fixed memory mode, then
>> + * MLO cannot be supported.
>> + */
>> + mem_node = ath12k_core_get_reserved_mem_by_name(ab, "mlo-global-mem");
>> + if (!mem_node)
>> + ab->single_chip_mlo_supp = false;
>> + else
>> + of_node_put(mem_node);
>> + }
>> +
>> + ret = ath12k_core_pre_init(ab);
>> + if (ret)
>> + goto err_core_free;
>> +
>> + ret = ath12k_ahb_resource_init(ab);
>> + if (ret)
>> + goto err_core_free;
>> +
>> + ret = ath12k_hal_srng_init(ab);
>> + if (ret)
>> + goto err_resource_deinit;
>> +
>> + ret = ath12k_ce_alloc_pipes(ab);
>> + if (ret) {
>> + ath12k_err(ab, "failed to allocate ce pipes: %d\n", ret);
>> + goto err_hal_srng_deinit;
>> + }
>> +
>> + ath12k_ahb_init_qmi_ce_config(ab);
>> +
>> + ret = ath12k_ahb_config_irq(ab);
>> + if (ret) {
>> + ath12k_err(ab, "failed to configure irq: %d\n", ret);
>> + goto err_ce_free;
>> + }
>> +
>> + ret = ath12k_core_init(ab);
>> + if (ret) {
>> + ath12k_err(ab, "failed to init core: %d\n", ret);
>> + goto err_ce_free;
>> + }
>> +
>> + return 0;
>> +
>> +err_ce_free:
>> + ath12k_ce_free_pipes(ab);
>> +
>> +err_hal_srng_deinit:
>> + ath12k_hal_srng_deinit(ab);
>> +
>> +err_resource_deinit:
>> + ath12k_ahb_resource_deinit(ab);
>> +
>> +err_core_free:
>> + ath12k_core_free(ab);
>> + platform_set_drvdata(pdev, NULL);
>> +
>> + return ret;
>> +}
>> +
>> +static void ath12k_ahb_remove_prepare(struct ath12k_base *ab)
>> +{
>> + unsigned long left;
>> +
>> + if (test_bit(ATH12K_FLAG_RECOVERY, &ab->dev_flags)) {
>> + left = wait_for_completion_timeout(&ab->driver_recovery,
>> + ATH12K_AHB_RECOVERY_TIMEOUT);
>> + if (!left)
>> + ath12k_warn(ab, "failed to receive recovery response completion\n");
>> + }
>> +
>> + set_bit(ATH12K_FLAG_UNREGISTERING, &ab->dev_flags);
>> + cancel_work_sync(&ab->restart_work);
>> + cancel_work_sync(&ab->qmi.event_work);
>> +}
>> +
>> +static void ath12k_ahb_free_resources(struct ath12k_base *ab)
>> +{
>> + struct platform_device *pdev = ab->pdev;
>> +
>> + ath12k_hal_srng_deinit(ab);
>> + ath12k_ce_free_pipes(ab);
>> + ath12k_ahb_resource_deinit(ab);
>> + ath12k_core_free(ab);
>> + platform_set_drvdata(pdev, NULL);
>> +}
>> +
>> +static void ath12k_ahb_remove(struct platform_device *pdev)
>> +{
>> + struct ath12k_base *ab = platform_get_drvdata(pdev);
>> +
>> + if (test_bit(ATH12K_FLAG_QMI_FAIL, &ab->dev_flags)) {
>> + ath12k_qmi_deinit_service(ab);
>> + goto qmi_fail;
>> + }
>> +
>> + ath12k_ahb_remove_prepare(ab);
>> + ath12k_core_deinit(ab);
>> +
>> +qmi_fail:
>> + ath12k_ahb_free_resources(ab);
>> +}
>> +
>> +static void ath12k_ahb_shutdown(struct platform_device *pdev)
>> +{
>> + struct ath12k_base *ab = platform_get_drvdata(pdev);
>> +
>> + /* platform shutdown() & remove() are mutually exclusive.
>> + * remove() is invoked during rmmod & shutdown() during
>> + * system reboot/shutdown.
>> + */
>
> You should rather explain why you cannot use one callback for both. Why
> this has to be duplicated?
>
>> + ath12k_ahb_remove_prepare(ab);
>> +
>> + if (!(test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)))
>> + goto free_resources;
>> +
>> + ath12k_core_deinit(ab);
>
> And why this is actually different order than remove().
>
The .shutdown() is not required for IPQ5332 wifi. In the next version
will remove the shutdown callback.
