Re: [PATCHv5 06/14] remoteproc/omap: Initialize and assign reserved memory node

[Andrew F. Davis]

On 1/30/20 2:55 PM, Suman Anna wrote:
> On 1/30/20 1:42 PM, Tero Kristo wrote:
>> On 30/01/2020 21:20, Andrew F. Davis wrote:
>>> On 1/30/20 2:18 PM, Tero Kristo wrote:
>>>> On 30/01/2020 20:11, Andrew F. Davis wrote:
>>>>> On 1/16/20 8:53 AM, Tero Kristo wrote:
>>>>>> From: Suman Anna <s-anna@xxxxxx>
>>>>>>
>>>>>> The reserved memory nodes are not assigned to platform devices by
>>>>>> default in the driver core to avoid the lookup for every platform
>>>>>> device and incur a penalty as the real users are expected to be
>>>>>> only a few devices.
>>>>>>
>>>>>> OMAP remoteproc devices fall into the above category and the OMAP
>>>>>> remoteproc driver _requires_ specific CMA pools to be assigned
>>>>>> for each device at the moment to align on the location of the
>>>>>> vrings and vring buffers in the RTOS-side firmware images. So,
>>>>>
>>>>>
>>>>> Same comment as before, this is a firmware issue for only some
>>>>> firmwares
>>>>> that do not handle being assigned vring locations correctly and instead
>>>>> hard-code them.
> 
> As for this statement, this can do with some updating. Post 4.20,
> because of the lazy allocation scheme used for carveouts including the
> vrings, the resource tables now have to use FW_RSC_ADDR_ANY and will
> have to wait for the vdev synchronization to happen.
> 
>>>>
>>>> I believe we discussed this topic in length in previous version but
>>>> there was no conclusion on it.
>>>>
>>>> The commit desc might be a bit misleading, we are not actually forced to
>>>> use specific CMA buffers, as we use IOMMU to map these to device
>>>> addresses. For example IPU1/IPU2 use internally exact same memory
>>>> addresses, iommu is used to map these to specific CMA buffer.
>>>>
>>>> CMA buffers are mostly used so that we get aligned large chunk of memory
>>>> which can be mapped properly with the limited IOMMU OMAP family of chips
>>>> have. Not sure if there is any sane way to get this done in any other
>>>> manner.
>>>>
>>>
>>>
>>> Why not use the default CMA area?
>>
>> I think using default CMA area getting the actual memory block is not
>> guaranteed and might fail. There are other users for the memory, and it
>> might get fragmented at the very late phase we are grabbing the memory
>> (omap remoteproc driver probe time.) Some chunks we need are pretty large.
>>
>> I believe I could experiment with this a bit though and see, or Suman
>> could maybe provide feedback why this was designed initially like this
>> and why this would not be a good idea.
> 
> I have given some explanation on this on v4 as well, but if it is not
> clear, there are restrictions with using default CMA. Default CMA has
> switched to be assigned from the top of the memory (higher addresses,
> since 3.18 IIRC), and the MMUs on IPUs and DSPs can only address
> 32-bits. So, we cannot blindly use the default CMA pool, and this will
> definitely not work on boards > 2 GB RAM. And, if you want to add in any
> firewall capability, then specific physical addresses becomes mandatory.
> 


If you need 32bit range allocations then
dma_set_mask(dev, DMA_BIT_MASK(32));

I'm not saying don't have support for carveouts, just make them
optional, keystone_remoteproc.c does this:

if (of_reserved_mem_device_init(dev))
	dev_warn(dev, "device does not have specific CMA pool\n");

There doesn't even needs to be a warning but that is up to you.

Andrew


> regards
> Suman
> 
>>
>> -Tero
>>
>>>
>>> Andrew
>>>
>>>
>>>> -Tero
>>>>
>>>>>
>>>>> This is not a requirement of the remote processor itself and so it
>>>>> should not fail to probe if a specific memory carveout isn't given.
>>>>>
>>>>>
>>>>>> use the of_reserved_mem_device_init/release() API appropriately
>>>>>> to assign the corresponding reserved memory region to the OMAP
>>>>>> remoteproc device. Note that only one region per device is
>>>>>> allowed by the framework.
>>>>>>
>>>>>> Signed-off-by: Suman Anna <s-anna@xxxxxx>
>>>>>> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
>>>>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>>>>>> ---
>>>>>> v5: no changes
>>>>>>
>>>>>> ÂÂ drivers/remoteproc/omap_remoteproc.c | 12 +++++++++++-
>>>>>> ÂÂ 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/remoteproc/omap_remoteproc.c
>>>>>> b/drivers/remoteproc/omap_remoteproc.c
>>>>>> index 0846839b2c97..194303b860b2 100644
>>>>>> --- a/drivers/remoteproc/omap_remoteproc.c
>>>>>> +++ b/drivers/remoteproc/omap_remoteproc.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>> ÂÂ #include <linux/module.h>
>>>>>> ÂÂ #include <linux/err.h>
>>>>>> ÂÂ #include <linux/of_device.h>
>>>>>> +#include <linux/of_reserved_mem.h>
>>>>>> ÂÂ #include <linux/platform_device.h>
>>>>>> ÂÂ #include <linux/dma-mapping.h>
>>>>>> ÂÂ #include <linux/remoteproc.h>
>>>>>> @@ -480,14 +481,22 @@ static int omap_rproc_probe(struct
>>>>>> platform_device *pdev)
>>>>>> ÂÂÂÂÂÂ if (ret)
>>>>>> ÂÂÂÂÂÂÂÂÂÂ goto free_rproc;
>>>>>> ÂÂ +ÂÂÂ ret = of_reserved_mem_device_init(&pdev->dev);
>>>>>> +ÂÂÂ if (ret) {
>>>>>> +ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "device does not have specific CMA
>>>>>> pool\n");
>>>>>> +ÂÂÂÂÂÂÂ goto free_rproc;
>>>>>> +ÂÂÂ }
>>>>>> +
>>>>>> ÂÂÂÂÂÂ platform_set_drvdata(pdev, rproc);
>>>>>> ÂÂ ÂÂÂÂÂ ret = rproc_add(rproc);
>>>>>> ÂÂÂÂÂÂ if (ret)
>>>>>> -ÂÂÂÂÂÂÂ goto free_rproc;
>>>>>> +ÂÂÂÂÂÂÂ goto release_mem;
>>>>>> ÂÂ ÂÂÂÂÂ return 0;
>>>>>> ÂÂ +release_mem:
>>>>>> +ÂÂÂ of_reserved_mem_device_release(&pdev->dev);
>>>>>> ÂÂ free_rproc:
>>>>>> ÂÂÂÂÂÂ rproc_free(rproc);
>>>>>> ÂÂÂÂÂÂ return ret;
>>>>>> @@ -499,6 +508,7 @@ static int omap_rproc_remove(struct
>>>>>> platform_device *pdev)
>>>>>> ÂÂ ÂÂÂÂÂ rproc_del(rproc);
>>>>>> ÂÂÂÂÂÂ rproc_free(rproc);
>>>>>> +ÂÂÂ of_reserved_mem_device_release(&pdev->dev);
>>>>>> ÂÂ ÂÂÂÂÂ return 0;
>>>>>> ÂÂ }
>>>>>>
>>>>
>>>> --Â
>>
>> -- 
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>