Hi, Krzysztof.
>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>Sent: Tuesday, February 11, 2025 2:38 AM
>To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
>hverkuil@xxxxxxxxx; sebastian.fricke@xxxxxxxxxxxxx; robh@xxxxxxxxxx;
>krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx
>Cc: linux-media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; linux-imx@xxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx; jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>;
>lafley.kim <lafley.kim@xxxxxxxxxxxxxxx>
>Subject: Re: [PATCH 7/8] media: chips-media: wave6: Add Wave6 control
>driver
>
>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.
I realize that my patch order was incorrect.
I will fix it and include the necessary kerneldoc in v2.
>
>> +
>> +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.
Thanks for the detailed explanation.
I will address this in v2.
>
>
>> + }
>> +
>> + 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?
I forgot to remove it.
I will address this in v2.
Thanks.
Nas.
>
>Best regards,
>Krzysztof
