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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/09/2024 17:51, Valentina.FernandezAlanis@microchip.com 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