Re: [RESEND PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Stephen Boyd <sboyd@codeaurora.org>]

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.

Are we gaining anything by having a remoteproc driver and device
for the video hardware here? Why can't the video driver
load/unload firmware with the mdt loader code by itself when it
wants to boot the processor? That seems like a much simpler
design and it nicely avoids this DT confusion.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project