Re: [PATCH v1 5/5] remoteproc: add support for Microchip IPC remoteproc platform driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/09/2024 19:00, Valentina Fernandez wrote:
> The Microchip family of RISC-V SoCs typically has one or more clusters.
> These clusters can be configured to run in Asymmetric Multi-Processing
> (AMP) mode.
> 
> Add a remoteproc platform driver to be able to load and boot firmware
> to the remote processor(s).

...

> +
> +static int mchp_ipc_rproc_prepare(struct rproc *rproc)
> +{
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +	struct device_node *np = priv->dev->of_node;
> +	struct rproc_mem_entry *mem;
> +	struct reserved_mem *rmem;
> +	struct of_phandle_iterator it;
> +	u64 device_address;
> +
> +	reinit_completion(&priv->start_done);
> +
> +	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> +	while (of_phandle_iterator_next(&it) == 0) {
> +		/*
> +		 * Ignore the first memory region which will be used vdev
> +		 * buffer. No need to do extra handlings, rproc_add_virtio_dev
> +		 * will handle it.
> +		 */
> +		if (!strcmp(it.node->name, "vdev0buffer"))

What? If you ignore the first, then why are you checking names? This
does not make sense. Especially that your binding did not say anything
about these phandles being somehow special.

> +			continue;
> +
> +		if (!strcmp(it.node->name, "rsc-table"))

Nope.

> +			continue;
> +
> +		rmem = of_reserved_mem_lookup(it.node);
> +		if (!rmem) {
> +			of_node_put(it.node);
> +			dev_err(priv->dev, "unable to acquire memory-region\n");
> +			return -EINVAL;
> +		}
> +
> +		device_address = rmem->base;
> +
> +		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)rmem->base,
> +					   rmem->size, device_address, mchp_ipc_rproc_mem_alloc,
> +					   mchp_ipc_rproc_mem_release, it.node->name);
> +
> +		if (!mem) {
> +			of_node_put(it.node);
> +			return -ENOMEM;
> +		}
> +
> +		rproc_add_carveout(rproc, mem);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mchp_ipc_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> +	int ret;
> +
> +	ret = rproc_elf_load_rsc_table(rproc, fw);
> +	if (ret)
> +		dev_info(&rproc->dev, "No resource table in elf\n");
> +
> +	return 0;
> +}
> +
> +static void mchp_ipc_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct mchp_ipc_rproc *priv = (struct mchp_ipc_rproc *)rproc->priv;
> +	struct mchp_ipc_msg msg;
> +	int ret;
> +
> +	msg.buf = (void *)&vqid;
> +	msg.size = sizeof(vqid);
> +
> +	ret = mbox_send_message(priv->mbox_channel, (void *)&msg);
> +	if (ret < 0)
> +		dev_err(priv->dev,
> +			"failed to send mbox message, status = %d\n", ret);
> +}
> +
> +static int mchp_ipc_rproc_attach(struct rproc *rproc)
> +{
> +	return 0;
> +}
> +
> +static struct resource_table
> +*mchp_ipc_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> +{
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +
> +	if (!priv->rsc_table)
> +		return NULL;
> +
> +	*table_sz = SZ_1K;
> +	return (struct resource_table *)priv->rsc_table;
> +}
> +
> +static const struct rproc_ops mchp_ipc_rproc_ops = {
> +	.prepare = mchp_ipc_rproc_prepare,
> +	.start = mchp_ipc_rproc_start,
> +	.get_loaded_rsc_table = mchp_ipc_rproc_get_loaded_rsc_table,
> +	.attach = mchp_ipc_rproc_attach,
> +	.stop = mchp_ipc_rproc_stop,
> +	.kick = mchp_ipc_rproc_kick,
> +	.load = rproc_elf_load_segments,
> +	.parse_fw = mchp_ipc_rproc_parse_fw,
> +	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> +	.sanity_check = rproc_elf_sanity_check,
> +	.get_boot_addr = rproc_elf_get_boot_addr,
> +};
> +
> +static int mchp_ipc_rproc_addr_init(struct mchp_ipc_rproc *priv,
> +				    struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	int i, err, rmem_np;
> +
> +	rmem_np = of_count_phandle_with_args(np, "memory-region", NULL);
> +	if (rmem_np <= 0)
> +		return 0;
> +
> +	for (i = 0; i < rmem_np; i++) {
> +		struct device_node *node;
> +		struct resource res;
> +
> +		node = of_parse_phandle(np, "memory-region", i);
> +		if (!node)
> +			continue;
> +
> +		if (!strncmp(node->name, "vdev", strlen("vdev"))) {

Uh? Why do you look for node names? What if I call the name differently?
Why would that matter?

> +			of_node_put(node);
> +			continue;
> +		}
> +
> +		if (!strcmp(node->name, "rsc-table")) {

No, really. Why are you checking for these?

NAK


> +			err = of_address_to_resource(node, 0, &res);
> +			if (err) {
> +				of_node_put(node);
> +				return dev_err_probe(dev, err,
> +						     "unable to resolve memory region\n");
> +			}
> +			priv->rsc_table = devm_ioremap(&pdev->dev,
> +						       res.start, resource_size(&res));
> +			of_node_put(node);
> +		}
> +	}
> +
> +	return 0;
> +}


Best regards,
Krzysztof