On 16/09/2024 21:18, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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. The idea in the code above is to skip the vdev buffer allocation and carveout registration. Later, when the remoteproc virtio driver registers the virtio device (rproc_add_virtio_dev), it will attempt to find the carveout. Since the carveout wasn’t registered, it will use the first memory region from the parent by calling of_reserved_mem_device_init_by_idx. This behavior is based on some existing platform drivers. However, upon further inspection, it seems that some newer drivers use rproc_of_resm_mem_entry_init to allocate vdev buffers. I will restructure this section and rephase/drop the comment. With regards the bindings, I'll explain better all the memory regions for v2. Just for everyone’s information, we have the following use cases: Early boot: Remote processors are booted by another entity before Linux, so we only need to attach. For this mode, we require the resource table as a memory region in the device tree. Late boot - Linux is responsible for loading the firmware and starting it on the remote processors. For this, we need the region used for the firmware image. In both cases, rpmsg communication is optional. This means that the vdev buffers and vrings memory regions are also optional. There could also be a mixed case where we start with early boot mode by attaching to an existing remoteproc, and then stop, start, and load another firmware once Linux has booted. In this case, we would require the resource table and firmware image region, and optionally, vdev buffers and vrings. > >> + continue; >> + >> + if (!strcmp(it.node->name, "rsc-table")) > > Nope. Since the resource table is only needed for early boot mode and does not need to be a carveout region, we are skipping that. I will work on making the resource table a fixed index in the memory-region property so that it doesn't have a fixed name. Thanks, Valentina > >> + 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 >