Hi, On 08/27/2016 01:23 AM, Bjorn Andersson wrote: > On Thu 25 Aug 04:10 PDT 2016, Stanimir Varbanov wrote: > >> Hi Bjorn, >> >> On 08/25/2016 03:05 AM, Bjorn Andersson wrote: >>> On Wed 24 Aug 08:36 PDT 2016, Stanimir Varbanov wrote: >>> >>>> Hi Rob, >>>> >>>> On 08/23/2016 08:32 PM, Rob Herring wrote: >>>>> On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote: >>>>>> Add devicetree binding document for Venus remote processor. >>>>>> >>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx> >>>>>> --- >>>>>> .../devicetree/bindings/remoteproc/qcom,venus.txt | 33 ++++++++++++++++++++++ >>>>>> 1 file changed, 33 insertions(+) >>>>>> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>>>> new file mode 100644 >>>>>> index 000000000000..06a2db60fa38 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>>>> @@ -0,0 +1,33 @@ >>>>>> +Qualcomm Venus Peripheral Image Loader >>>>>> + >>>>>> +This document defines the binding for a component that loads and boots firmware >>>>>> +on the Qualcomm Venus remote processor core. >>>>> >>>>> This does not make sense to me. Venus is the video encoder/decoder h/w, >>>>> right? Why is the firmware loader separate from the codec block? Why >>>>> rproc is used? Are there multiple clients? Naming it rproc_venus implies >>>>> there aren't. And why does the firmware loading need 8MB of memory at a >>>>> fixed address? >>>>> >>>> >>>> The firmware for Venus case is 5MB. And here is 8MB because of >>>> dma_alloc_from_coherent size restriction. >>>> >>> >>> Then you should specify it 5MB large and we'll have to deal with this >>> implementation issue in the code. I've created a JIRA ticket for >>> the dma_alloc_from_coherent() behavior. >> >> Infact it should be 5MB + ~100KB for iommu page table. >> > > Trying to wrap my head around how the iommu part works here. The > downstream code seems to indicate that this is a "generic" secure iommu > interface - used by venus, camera and kgsl; likely for dealing with DRM > protected buffers. The secure iommu interface is for content protected buffers. But these secure iommu contexts aren't used by msm DRM nor Venus in mainline. In Venus case I use non-secure iommu context for data buffers. > > As such the iommu tables are not part of the venus rproc; I believe they > should either be tied into the msm-iommu driver or perhaps exposed as > its own iommu(?). The page tables are in msm-iommu driver. > > > But I presume from your inclusion that you've concluded that the venus > firmware we have refuses to execute without these tables at least > initialized, is this correct? Yes, the SMC call for PAS memory-setup will fail if this page table is not initialized. > >>> >>>> The address is not really fixed, cause the firmware could support >>>> relocation. In this example I just picked up the next free memory region >>>> in memory-reserved from msm8916.dtsi. >>>> >>> >>> In 8974 we do have a physical region where it's expected to be loaded. >>> >>> So in line with upcoming remoteproc work we should support referencing a >>> reserved-memory node with either reg or size. >>> >>> In the case of spotting a "reg" we're currently better off using >>> ioremap. We're looking at getting the remoteproc core to deal with this >>> mess. >> >> You mean that remoteproc core will parse memory-region property? >> > > It has to, because it's a quite common scenario for remoteproc drivers > to either get its backing memory from a static region or be restricted > to part of system ram - properties that reserved-memory and > memory-region captures already. OK, I have no issues with that. My concern is the manual parsing of 'memory-region' and 'reg' properties in remoteproc core. So that idea is to have generic binding for rproc, that would be good. > >>> >>> >>> So, on 8916 I think you should use the form: >>> >>> venus_mem: venus { >>> size = <0x500000>; >>> }; >> >> Don't forget that the physical address where the firmware is stored has >> some range, the scm call will fail if it is out of the expected range, >> probably because of some security reasons. So maybe alloc-ranges should >> be specified here. >> > > Thanks for highlighting this. > >>> >>> And I don't think you should use the shared-dma-pool compatible, because >>> this is not a region for multiple devices to allocate dma memory out of. >> >> Then I cannot reuse reserved-mem infrastructure. >> > > You're right. If I understand the code correctly we need to use the > compatible shared-dma-pool and mark it either "no-map" or "reusable", to > be able to use dma_alloc_coherent(). correct. > > > But I presume we have the implementation issue of dma_alloc_coherent() > failing in either case with the 5MB size. I think we need to look into I'd be good to include Marek Szyprowski? At least he will know what design restrictions there are. > that - and have created a JIRA ticket for it. > > Regards, > Bjorn > -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html