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

[Andrew F. Davis]

On 1/30/20 3:39 PM, Suman Anna wrote:
> On 1/30/20 2:22 PM, Andrew F. Davis wrote:
>> 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.
> 
> It is not exactly an apples to apples comparison. K2s do not have MMUs,
> and most of our firmware images on K2 are actually running out of the
> DSP internal memory.
> 


So again we circle back to it being a firmware issue, if K2 can get away
without needing carveouts and it doesn't even have an MMU then certainly
OMAP/DRA7x class devices can handle it even better given they *do* have
an IOMMU. Unless someone is hard-coding the IOMMU configuration.. In
which case we are still just hacking around the problem here with
mandatory specific address memory carveouts.

Andrew


> regards
> Suman
> 
>>
>> 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
>>>
>