Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <83b1b1b9-377a-c53b-13dc-12c8095eb99b@quicinc.com>
Date:   Wed, 19 Jul 2023 21:02:33 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
Subject: Re: [V3,09/11] remoteproc: qcom: Add Hexagon based multipd rproc
 driver
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <mathieu.poirier@linaro.org>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>, <mturquette@baylibre.com>,
        <sboyd@kernel.org>, <quic_eberman@quicinc.com>, <kvalo@kernel.org>,
        <loic.poulain@linaro.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org>
CC:     <quic_srichara@quicinc.com>, <quic_sjaganat@quicinc.com>,
        <quic_kathirav@quicinc.com>, <quic_anusha@quicinc.com>
References: <20230718120501.3205661-1-quic_mmanikan@quicinc.com>
 <20230718120501.3205661-10-quic_mmanikan@quicinc.com>
 <fedea75a-a5f1-fbe4-7f12-75bfdf9bfcf2@linaro.org>
From:   Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
In-Reply-To: <fedea75a-a5f1-fbe4-7f12-75bfdf9bfcf2@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/19/2023 12:56 PM, Krzysztof Kozlowski wrote:
> On 18/07/2023 14:04, Manikanta Mylavarapu wrote:
>> It adds support to bring up remoteproc's on multipd model.
>> Pd means protection domain. It's similar to process in Linux.
>> Here QDSP6 processor runs each wifi radio functionality on a
>> separate process. One process can't access other process
>> resources, so this is termed as PD i.e protection domain.
>>
>> Here we have two pd's called root and user pd. We can correlate
>> Root pd as root and user pd as user in linux. Root pd has more
>> privileges than user pd. Root will provide services to user pd.
>>
> 
> 
>> +static int q6_get_inbound_irq(struct qcom_q6v5 *q6,
>> +			      struct platform_device *pdev,
>> +			      const char *int_name,
>> +			      int index, int *pirq,
>> +			      irqreturn_t (*handler)(int irq, void *data))
>> +{
>> +	int ret, irq;
>> +	char *interrupt, *tmp = (char *)int_name;
>> +	struct q6_wcss *wcss = q6->rproc->priv;
>> +
>> +	irq = platform_get_irq(pdev, index);
>> +	if (irq < 0) {
>> +		if (irq != -EPROBE_DEFER)
>> +			dev_err_probe(&pdev->dev, irq,
>> +				      "failed to retrieve %s IRQ: %d\n",
>> +				      int_name, irq);
> 
> Wait, what? This does not make any sense. dev_err_probe is to replace
> all this dance. return dev_err_probe which I explicitly asked last time:
>  > 
https://lore.kernel.org/all/2061a641-4b97-1aa6-27cd-99f01a785033@linaro.org/
> 
Yeah i got your point. I will return dev_err_probe alone.

>> +		return irq;
>> +	}
>> +
>> +	*pirq = irq;
>> +
>> +	interrupt = devm_kzalloc(&pdev->dev, BUF_SIZE, GFP_KERNEL);
>> +	if (!interrupt)
>> +		return -ENOMEM;
>> +
>> +	snprintf(interrupt, BUF_SIZE, "q6v5_wcss_userpd%d_%s", wcss->pd_asid, tmp);
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, *pirq,
>> +					NULL, handler,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					interrupt, q6);
>> +	if (ret) {
>> +		dev_err_probe(&pdev->dev, ret,
>> +			      "failed to acquire %s irq\n", interrupt);
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int q6_get_outbound_irq(struct qcom_q6v5 *q6,
>> +			       struct platform_device *pdev,
>> +			       const char *int_name)
>> +{
>> +	struct qcom_smem_state *tmp_state;
>> +	unsigned  bit;
>> +
>> +	tmp_state = qcom_smem_state_get(&pdev->dev, int_name, &bit);
>> +	if (IS_ERR(tmp_state)) {
>> +		dev_err_probe(&pdev->dev, IS_ERR(tmp_state),
>> +			      "failed to acquire %s state\n", int_name);
>> +		return PTR_ERR(tmp_state);
> 
> So it is everywhere...
> 
> 
Wherever applicable i will return dev_err_probe.

>> +	}
>> +
>> +	if (!strcmp(int_name, "stop")) {
>> +		q6->state = tmp_state;
>> +		q6->stop_bit = bit;
>> +	} else if (!strcmp(int_name, "spawn")) {
>> +		q6->spawn_state = tmp_state;
>> +		q6->spawn_bit = bit;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int init_irq(struct qcom_q6v5 *q6,
>> +		    struct platform_device *pdev, struct rproc *rproc,
>> +		    int crash_reason, const char *load_state,
>> +		    void (*handover)(struct qcom_q6v5 *q6))
>> +{
>> +	int ret;
>> +	struct q6_wcss *wcss = rproc->priv;
>> +
>> +	q6->rproc = rproc;
>> +	q6->dev = &pdev->dev;
>> +	q6->crash_reason = crash_reason;
>> +	q6->handover = handover;
>> +
>> +	init_completion(&q6->start_done);
>> +	init_completion(&q6->stop_done);
>> +	init_completion(&q6->spawn_done);
>> +
>> +	ret = q6_get_outbound_irq(q6, pdev, "stop");
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = q6_get_outbound_irq(q6, pdev, "spawn");
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Get pd_asid to prepare interrupt names */
>> +	wcss->pd_asid = qcom_get_pd_asid(rproc);
>> +
>> +	ret = q6_get_inbound_irq(q6, pdev, "fatal", 0, &q6->fatal_irq,
>> +				 q6v5_fatal_interrupt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = q6_get_inbound_irq(q6, pdev, "ready", 1, &q6->ready_irq,
>> +				 q6v5_ready_interrupt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = q6_get_inbound_irq(q6, pdev, "stop-ack", 3, &q6->stop_irq,
>> +				 q6v5_stop_interrupt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = q6_get_inbound_irq(q6, pdev, "spawn-ack", 2, &q6->spawn_irq,
>> +				 q6v5_spawn_interrupt);
>> +	if (ret)
>> +		return ret;
>> +	return 0;
>> +}
>> +
>> +static void q6_release_resources(struct platform_device *pdev)
>> +{
>> +	struct rproc *upd_rproc;
>> +	struct device_node *upd_np;
>> +	struct platform_device *upd_pdev;
>> +
>> +	/* Release userpd resources */
>> +	for_each_available_child_of_node(pdev->dev.of_node, upd_np) {
> 
> You should not iterate over OF to get devices to unregister. What if you
> have more nodes than before because of overlay?
> 
> 
>> +		upd_pdev = of_find_device_by_node(upd_np);
>> +		if (!upd_pdev)
>> +			continue;
>> +
>> +		upd_rproc = platform_get_drvdata(upd_pdev);
>> +		if (!upd_rproc) {
>> +			platform_device_unregister(upd_pdev);
>> +			continue;
>> +		}
>> +
>> +		rproc_del(upd_rproc);
>> +		rproc_free(upd_rproc);
>> +	}
>> +}
>> +
>> +static int q6_register_userpd(struct platform_device *pdev)
>> +{
>> +	struct q6_wcss *wcss;
>> +	struct rproc *rproc = NULL;
>> +	int ret;
>> +	struct device_node *userpd_np;
>> +	struct platform_device *userpd_pdev;
>> +	const char *firmware_name = NULL;
>> +
>> +	for_each_available_child_of_node(pdev->dev.of_node, userpd_np) {
>> +		ret = of_property_read_string(userpd_np, "firmware-name",
>> +					      &firmware_name);
>> +		if (ret < 0)
>> +			continue;
>> +
>> +		dev_info(&pdev->dev, "%s node found\n", userpd_np->name);
>> +
>> +		userpd_pdev = of_platform_device_create(userpd_np,
>> +							userpd_np->name,
>> +							&pdev->dev);
>> +		if (!userpd_pdev) {
>> +			ret = -ENODEV;
>> +			dev_err_probe(&pdev->dev, ret,
>> +				      "failed to create %s platform device\n",
>> +				      userpd_np->name);
>> +			goto release_resource;
>> +		}
>> +		userpd_pdev->dev.driver = pdev->dev.driver;
>> +		rproc = rproc_alloc(&userpd_pdev->dev, userpd_pdev->name,
>> +				    &wcss_ops, firmware_name,
>> +				    sizeof(*wcss));
>> +		if (!rproc) {
>> +			ret = -ENOMEM;
>> +			goto release_resource;
>> +		}
>> +
>> +		wcss = rproc->priv;
>> +		wcss->dev = &userpd_pdev->dev;
>> +
>> +		ret = q6_alloc_memory_region(wcss);
>> +		if (ret)
> 
> How do you release the resource allocated in rproc_alloc() for this
> node? drvdata is not set, so your cleanup function will skip it.
> 
>> +			goto release_resource;
>> +
>> +		ret = init_irq(&wcss->q6, userpd_pdev, rproc,
>> +			       WCSS_CRASH_REASON, NULL, NULL);
>> +		if (ret)
>> +			goto release_resource;
>> +
>> +		rproc->auto_boot = false;
>> +		ret = rproc_add(rproc);
>> +		if (ret)
>> +			goto release_resource;
>> +
>> +		platform_set_drvdata(userpd_pdev, rproc);
>> +		qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, userpd_pdev->name);
>> +	}
>> +	return 0;
>> +
>> +release_resource:
>> +	q6_release_resources(pdev);
>> +	return ret;
>> +}
>> +
>> +static int q6_wcss_probe(struct platform_device *pdev)
>> +{
>> +	const struct wcss_data *desc;
>> +	struct q6_wcss *wcss;
>> +	struct rproc *rproc;
>> +	int ret;
>> +	char *subdev_name;
>> +	const char **firmware;
>> +
>> +	desc = of_device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>> +	firmware = devm_kcalloc(&pdev->dev, MAX_FIRMWARE,
>> +				sizeof(*firmware), GFP_KERNEL);
>> +	if (!firmware)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_string_array(pdev->dev.of_node, "firmware-name",
>> +					    firmware, MAX_FIRMWARE);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops,
>> +			    firmware[0], sizeof(*wcss));
>> +	if (!rproc)
>> +		return -ENOMEM;
>> +
>> +	wcss = rproc->priv;
>> +	wcss->dev = &pdev->dev;
>> +	wcss->desc = desc;
>> +	wcss->firmware = firmware;
>> +
>> +	ret = q6_alloc_memory_region(wcss);
>> +	if (ret)
>> +		goto free_rproc;
>> +
>> +	ret = desc->init_irq(&wcss->q6, pdev, rproc,
>> +			     desc->crash_reason_smem, NULL, NULL);
>> +	if (ret)
>> +		goto free_rproc;
>> +
>> +	if (desc->glink_subdev_required)
>> +		qcom_add_glink_subdev(rproc, &wcss->glink_subdev, desc->ssr_name);
>> +
>> +	subdev_name = (char *)(desc->ssr_name ? desc->ssr_name : pdev->name);
> 
> Wrong cast. Why are you dropping const? That's a bug.
> 

Yeah, I will use desc->ssr_name itself.

>> +	qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, subdev_name);
>> +
>> +	rproc->auto_boot = false;
>> +	ret = rproc_add(rproc);
>> +	if (ret)
>> +		goto free_rproc;
>> +
>> +	platform_set_drvdata(pdev, rproc);
>> +
>> +	ret = q6_register_userpd(pdev);
>> +	if (ret) {
>> +		dev_err_probe(&pdev->dev, ret, "Failed to register userpd\n");
>> +		return ret;
> 
> return dev_err_probe
> 
ok, sure.

>> +	}
>> +
>> +	return 0;
>> +
>> +free_rproc:
>> +	rproc_free(rproc);
>> +
>> +	return ret;
> 

Thanks & Regards,
Manikanta.