RE: [PATCH 7/8] media: chips-media: wave6: Add Wave6 control driver

[Nas Chung <nas.chung@chipsnmedia.com>]

Hi, Krzysztof.

>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@kernel.org>
>Sent: Tuesday, February 11, 2025 2:38 AM
>To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org;
>hverkuil@xs4all.nl; sebastian.fricke@collabora.com; robh@kernel.org;
>krzk+dt@kernel.org; conor+dt@kernel.org
>Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-imx@nxp.com; linux-arm-
>kernel@lists.infradead.org; jackson.lee <jackson.lee@chipsnmedia.com>;
>lafley.kim <lafley.kim@chipsnmedia.com>
>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