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
