On 30/07/2025 11:35, Krzysztof Kozlowski wrote:
> On 30/07/2025 11:27, Junhui Liu wrote:
>> On 30/07/2025 08:46, Krzysztof Kozlowski wrote:
>>> On 28/07/2025 13:03, Junhui Liu wrote:
>>>> +
>>>> +static int cv1800b_c906l_mem_alloc(struct rproc *rproc,
>>>> + struct rproc_mem_entry *mem)
>>>> +{
>>>> + void __iomem *va;
>>>> +
>>>> + va = ioremap_wc(mem->dma, mem->len);
>>>> + if (!va)
>>>> + return -ENOMEM;
>>>> +
>>>> + /* Update memory entry va */
>>>> + mem->va = (void *)va;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int cv1800b_c906l_mem_release(struct rproc *rproc,
>>>> + struct rproc_mem_entry *mem)
>>>> +{
>>>> + iounmap((void __iomem *)mem->va);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int cv1800b_c906l_add_carveout(struct rproc *rproc)
>>>> +{
>>>> + struct device *dev = rproc->dev.parent;
>>>> + struct device_node *np = dev->of_node;
>>>> + struct of_phandle_iterator it;
>>>> + struct rproc_mem_entry *mem;
>>>> + struct reserved_mem *rmem;
>>>> + int i = 0;
>>>> +
>>>> + /* Register associated reserved memory regions */
>>>> + of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>>>> + while (of_phandle_iterator_next(&it) == 0) {
>>>> + rmem = of_reserved_mem_lookup(it.node);
>>>> + if (!rmem) {
>>>> + of_node_put(it.node);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (!strcmp(it.node->name, "vdev0buffer")) {
>>>
>>> Why are you adding undocumented ABI? And so hidden, not even using
>>> standard OF API!
>>>
>>> How does this behaves when I change your DTS to call it
>>> "whateverbuffer"? Does it work? Obviously not.
>>>
>>> No, stop doing that.
>>
>> Yes, you're right. I will consider introducing a "memory-region-names"
>> property in the bindings, instead of relying on the node labels directly.
>
>
> You don't need it. First, you use some old code as template, but you
> should look how or re-use Rob's code rewriting this completely.
Sorry, I didn't catch up with that patch. I will look into it and update
my implementation accordingly.
>
> Second, list has strict order, so you know exactly where the vdev0
> buffer is. It cannot be on any other position of the list.
Thanks for the advice, I will use the order in the list to identify the
memory region.
>
> This is why you define the ABI. Use then the ABI.
>
Understood, I will reconsider this.
--
Best regards,
Junhui Liu
