From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [RESEND PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform. Date: Fri, 16 Dec 2016 23:11:57 -0800 Message-ID: <20161217071157.GU3439@tuxbot> References: <1480416647-3518-1-git-send-email-akdwived@codeaurora.org> <1480416647-3518-2-git-send-email-akdwived@codeaurora.org> <20161129192702.GA6095@codeaurora.org> <1c22ad41-2d14-be04-ecf0-f0395463978a@codeaurora.org> <20161213065118.GF3439@tuxbot> <20161217005608.GU5423@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pg0-f41.google.com ([74.125.83.41]:34201 "EHLO mail-pg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750727AbcLQHMD (ORCPT ); Sat, 17 Dec 2016 02:12:03 -0500 Received: by mail-pg0-f41.google.com with SMTP id a1so17519600pgf.1 for ; Fri, 16 Dec 2016 23:12:02 -0800 (PST) Content-Disposition: inline In-Reply-To: <20161217005608.GU5423@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Stephen Boyd Cc: Stanimir Varbanov , "Dwivedi, Avaneesh Kumar (avani)" , agross@codeaurora.org, linux-arm-msm@vger.kernel.org On Fri 16 Dec 16:56 PST 2016, Stephen Boyd wrote: > On 12/12, Bjorn Andersson wrote: > > On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote: > > [..] > > > > > I think using power domain in remoteproc driver will break power > > > management of the v4l2 venus driver. Presently I use runtime pm to > > > switch ON/OFF GDSC and enable/disable Venus clocks. > > > > > > When the .probe of venus remoteproc driver is called the power domain > > > will become ON and never switched OFF (except when platform driver > > > .remove method is called). One possible solution would be to add runtime > > > pm in venus remoreproc driver. > > > > > > IMO we should think more about that. > > > > > > Bjorn, Stephen what are your opinions? > > > > > > > Sorry for not getting back to you on this after our earlier discussion > > on the topic. > > > > Modelling the remoteproc and codec drivers as completely separate > > entities (with only the rproc phandle) gives us issues with the timing > > between the two. As I pulled down and started playing with [1] I noticed > > that I was not able to get the codec driver to probe unless I compiled > > it as a module which I insmoded after my /lib/firmware became available. > > > > In addition to this, there's no way for the remoteproc driver to inform > > the codec driver when it's actually shutting down and booting back up > > during a crash recovery (an async operation). > > > > And lastly when I talked to Rob about the DT binding the obvious > > question of why one piece of hardware have two disjunct nodes in the DT. > > > > > > So I believe we should represent the codec driver as a child of the > > remoteproc driver, utilizing the newly introduced "remoteproc > > subdevices" to probe and remove the codec driver based on the state of > > the remoteproc driver. This relationship will also allow us to tie > > certain resources (e.g. the clocks and power-domain) to the remoteproc > > driver and use pm_runtime in either driver to make sure the resources > > are enabled appropriately. > > > > I did backport some patches from my v4.10 remoteproc pull request into > > [2] and merged this into [1] and made a prototype of this. You can find > > it here [3], I did test that I can boot the remoteproc and run > > v4l2-compliance, shut it down, boot it back up and re-run > > v4l2-compliance, and it seems to work fine. > > > > From a DT perspective this seems backwards. We shouldn't be > putting something with a reg property underneath a "virtual" node > that doesn't have a reg property. It's already causing pain, > evident by these sorts of patches, so something seems wrong. > I agree. > Are we gaining anything by having a remoteproc driver and device > for the video hardware here? After discussing the matter with Avaneesh I realized that the downstream driver powers the venus core up and down based on the presence of clients. I expect that this means we have to do the same to be able to meet our power KPIs. With this in mind the remoteproc driver becomes a wrapper for a mdt loader and the scm pas interface - and brings with it a bunch of other things, expectations and challenges. > Why can't the video driver load/unload firmware with the mdt loader > code by itself when it wants to boot the processor? Providing a saner api around the mdt-loader and pas would allow venus and Adreno to reuse the code, without the remoteproc bloat. > That seems like a much simpler design and it nicely avoids this DT > confusion. > Agreed. I was under the impression that the venus core was always on, but after re-reading the downstream venus code a few more times I think this would be much cleaner to do so, both implementation and binding wise. Regards, Bjorn