On 18/09/2024 17:51, Valentina.FernandezAlanis@xxxxxxxxxxxxx wrote:
> 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.
The list of memory-regions already HAS fixed indices. All this is not
only confusing, but incorrect. I commented that if I call the node
"rsc-not-a-table" your code will stop working.
Best regards,
Krzysztof
