On 10/02/2025 10:07, Nas Chung wrote:
> +
> +int wave6_vpu_ctrl_resume_and_get(struct device *dev, struct wave6_vpu_entity *entity)
> +{
> + struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> + bool boot;
> + int ret;
> +
> + if (!ctrl)
> + return -EINVAL;
> +
> + if (!entity || !entity->dev || !entity->read_reg || !entity->write_reg)
> + return -EINVAL;
> +
> + mutex_lock(&ctrl->ctrl_lock);
> +
> + ret = pm_runtime_resume_and_get(ctrl->dev);
> + if (ret) {
> + dev_err(dev, "pm runtime resume fail, ret = %d\n", ret);
> + mutex_unlock(&ctrl->ctrl_lock);
> + return ret;
> + }
> +
> + entity->booted = false;
> +
> + if (ctrl->current_entity)
> + boot = false;
> + else
> + boot = list_empty(&ctrl->entities) ? true : false;
> +
> + list_add_tail(&entity->list, &ctrl->entities);
> + if (boot)
> + ret = wave6_vpu_ctrl_try_boot(ctrl, entity);
> +
> + if (ctrl->state == WAVE6_VPU_STATE_ON)
> + wave6_vpu_ctrl_on_boot(entity);
> +
> + if (ret)
> + pm_runtime_put_sync(ctrl->dev);
> +
> + mutex_unlock(&ctrl->ctrl_lock);
> +
> + return ret;
> +}
Drop entire function.
> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_resume_and_get);
Drop.
> +
> +void wave6_vpu_ctrl_put_sync(struct device *dev, struct wave6_vpu_entity *entity)
> +{
> + struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> + if (!ctrl)
> + return;
> +
> + if (entity == ctrl->current_entity)
> + wave6_vpu_ctrl_wait_done(dev);
> +
> + mutex_lock(&ctrl->ctrl_lock);
> +
> + if (!wave6_vpu_ctrl_find_entity(ctrl, entity))
> + goto exit;
> +
> + list_del_init(&entity->list);
> + if (list_empty(&ctrl->entities)) {
> + if (!entity->read_reg(entity->dev, W6_VPU_VCPU_CUR_PC))
> + wave6_vpu_ctrl_set_state(ctrl, WAVE6_VPU_STATE_OFF);
> + else
> + wave6_vpu_ctrl_sleep(ctrl, entity);
> + }
> +
> + if (!pm_runtime_suspended(ctrl->dev))
> + pm_runtime_put_sync(ctrl->dev);
> +exit:
> + mutex_unlock(&ctrl->ctrl_lock);
> +}
> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_put_sync);
Drop entire function. Dead code.
... or you arranged your patches really incorrectly, but this I can't
really judge.
> +
> +int wave6_vpu_ctrl_wait_done(struct device *dev)
> +{
> + struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!ctrl)
> + return -EINVAL;
> +
> + if (ctrl->state == WAVE6_VPU_STATE_OFF)
> + return -EINVAL;
> +
> + if (ctrl->state == WAVE6_VPU_STATE_ON)
> + return 0;
> +
> + ret = wave6_wait_event_freezable_timeout(ctrl->load_fw_wq,
> + wave6_vpu_ctrl_get_state(dev) ==
> + WAVE6_VPU_STATE_ON,
> + msecs_to_jiffies(W6_BOOT_WAIT_TIMEOUT));
> + if (ret == -ERESTARTSYS || !ret) {
> + dev_err(ctrl->dev, "fail to wait vcpu boot done,ret %d\n", ret);
> + mutex_lock(&ctrl->ctrl_lock);
> + wave6_vpu_ctrl_set_state(ctrl, WAVE6_VPU_STATE_OFF);
> + mutex_unlock(&ctrl->ctrl_lock);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&ctrl->ctrl_lock);
> + wave6_vpu_ctrl_boot_done(ctrl, 0);
> + mutex_unlock(&ctrl->ctrl_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_wait_done);
There is no user of this outside. Drop.
> +
> +int wave6_vpu_ctrl_get_state(struct device *dev)
> +{
> + struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> + if (!ctrl)
> + return -EINVAL;
> +
> + return ctrl->state;
> +}
> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_get_state);
There is no user of this outside. Drop.
Whatever export stays, must be used by a following patch. You *must* add
the kerneldoc for it.
> +
> +static void wave6_vpu_ctrl_init_reserved_boot_region(struct vpu_ctrl *ctrl)
> +{
> + if (ctrl->boot_mem.size < WAVE6_CODE_BUF_SIZE) {
> + dev_warn(ctrl->dev, "boot memory size (%zu) is too small\n", ctrl->boot_mem.size);
> + ctrl->boot_mem.phys_addr = 0;
> + ctrl->boot_mem.size = 0;
> + memset(&ctrl->boot_mem, 0, sizeof(ctrl->boot_mem));
> + return;
> + }
> +
> + ctrl->boot_mem.vaddr = devm_memremap(ctrl->dev,
> + ctrl->boot_mem.phys_addr,
> + ctrl->boot_mem.size,
> + MEMREMAP_WC);
> + if (!ctrl->boot_mem.vaddr) {
> + memset(&ctrl->boot_mem, 0, sizeof(ctrl->boot_mem));
> + return;
> + }
> +
> + ctrl->boot_mem.dma_addr = dma_map_resource(ctrl->dev,
> + ctrl->boot_mem.phys_addr,
> + ctrl->boot_mem.size,
> + DMA_BIDIRECTIONAL,
> + 0);
> + if (!ctrl->boot_mem.dma_addr) {
> + memset(&ctrl->boot_mem, 0, sizeof(ctrl->boot_mem));
> + return;
> + }
> +
> + dev_info(ctrl->dev, "boot phys_addr: %pad, dma_addr: %pad, size: 0x%zx\n",
> + &ctrl->boot_mem.phys_addr,
> + &ctrl->boot_mem.dma_addr,
> + ctrl->boot_mem.size);
No, drop. Reasoning further.
> +}
...
> +
> + ctrl->num_clks = ret;
> +
> + np = of_parse_phandle(pdev->dev.of_node, "boot", 0);
> + if (np) {
> + struct resource mem;
> +
> + ret = of_address_to_resource(np, 0, &mem);
> + of_node_put(np);
> + if (!ret) {
> + ctrl->boot_mem.phys_addr = mem.start;
> + ctrl->boot_mem.size = resource_size(&mem);
> + wave6_vpu_ctrl_init_reserved_boot_region(ctrl);
> + } else {
> + dev_warn(&pdev->dev, "boot resource is not available.\n");
> + }
> + }
> +
> + ctrl->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> + if (ctrl->sram_pool) {
> + ctrl->sram_buf.size = ctrl->res->sram_size;
> + ctrl->sram_buf.vaddr = gen_pool_dma_alloc(ctrl->sram_pool,
> + ctrl->sram_buf.size,
> + &ctrl->sram_buf.phys_addr);
> + if (!ctrl->sram_buf.vaddr)
> + ctrl->sram_buf.size = 0;
> + else
> + ctrl->sram_buf.dma_addr = dma_map_resource(&pdev->dev,
> + ctrl->sram_buf.phys_addr,
> + ctrl->sram_buf.size,
> + DMA_BIDIRECTIONAL,
> + 0);
> +
> + dev_info(&pdev->dev, "sram 0x%pad, 0x%pad, size 0x%lx\n",
> + &ctrl->sram_buf.phys_addr, &ctrl->sram_buf.dma_addr, ctrl->sram_buf.size);
You are not supposed to print addresses. This look like fixed hardware
mappings, so probably harmless but dma_addr not. Drop entire dev_info,
this is really discouraged practice.
> + }
> +
> + if (of_find_property(pdev->dev.of_node, "support-follower", NULL))
> + ctrl->support_follower = true;
> +
> + wave6_cooling_init(ctrl);
> +
> + pm_runtime_enable(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static void wave6_vpu_ctrl_remove(struct platform_device *pdev)
> +{
> + struct vpu_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> + wave6_vpu_ctrl_clear_buffers(ctrl);
> + wave6_cooling_remove(ctrl);
> + if (ctrl->sram_pool && ctrl->sram_buf.vaddr) {
> + dma_unmap_resource(&pdev->dev,
> + ctrl->sram_buf.dma_addr,
> + ctrl->sram_buf.size,
> + DMA_BIDIRECTIONAL,
> + 0);
> + gen_pool_free(ctrl->sram_pool,
> + (unsigned long)ctrl->sram_buf.vaddr,
> + ctrl->sram_buf.size);
> + }
> + if (ctrl->boot_mem.dma_addr)
> + dma_unmap_resource(&pdev->dev,
> + ctrl->boot_mem.dma_addr,
> + ctrl->boot_mem.size,
> + DMA_BIDIRECTIONAL,
> + 0);
> + mutex_destroy(&ctrl->ctrl_lock);
> +}
> +
> +#ifdef CONFIG_PM
> +static int wave6_vpu_ctrl_runtime_suspend(struct device *dev)
> +{
> + struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> + clk_bulk_disable_unprepare(ctrl->num_clks, ctrl->clks);
> + return 0;
> +}
> +
> +static int wave6_vpu_ctrl_runtime_resume(struct device *dev)
> +{
> + struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> + return clk_bulk_prepare_enable(ctrl->num_clks, ctrl->clks);
> +}
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int wave6_vpu_ctrl_suspend(struct device *dev)
> +{
> + return 0;
Why do you need empty callbacks?
Best regards,
Krzysztof
