From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document Date: Tue, 30 Aug 2016 10:17:42 -0700 Message-ID: <20160830171742.GN15161@tuxbot> References: <1471622000-1906-1-git-send-email-stanimir.varbanov@linaro.org> <1471622000-1906-4-git-send-email-stanimir.varbanov@linaro.org> <20160823173259.GA13327@rob-hp-laptop> <20160825000521.GH15161@tuxbot> <3429173a-d55a-51e1-0973-7e5bd31be297@linaro.org> <20160826222345.GK15161@tuxbot> <5afd7f2f-ec1b-d2ce-b833-81df010e24de@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <5afd7f2f-ec1b-d2ce-b833-81df010e24de@linaro.org> Sender: linux-kernel-owner@vger.kernel.org To: Stanimir Varbanov Cc: Rob Herring , Andy Gross , Ohad Ben-Cohen , Stephen Boyd , Mark Rutland , Rob Clark , Srinivas Kandagatla , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org List-Id: devicetree@vger.kernel.org On Mon 29 Aug 04:48 PDT 2016, Stanimir Varbanov wrote: [..] > > 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. > We must consider the case when DRM, VFE and Venus handles protected 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. > So, just to verify your answer, the msm-iommu driver will handle both protected and unprotected? > > > > > > 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. > If the msm-iommu driver will handle the protected buffers (or if there will be a separate iommu driver for protected buffers) it should issue these calls, to not be dependant on the rproc-venus driver. With that I think we should make the rproc-venus driver depend on this being setup (even if this means creating a "dummy" driver for the protected iommu handling for now). > > > >>> > >>>> 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. > I do share your concerns here. But it's a recurring issue with remoteproc drivers. [..] > > 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. > Please do. The more I look at this the more I think we must use the existing infrastructure for allocating "dma memory". Getting dma_alloc_coherent() supporting non-power-of-2 memory regions would allow us to use the existing infrastructure, for both fixed and dynamically placed memory carveouts in remoteproc. Regards, Bjorn