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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote:
[..]
> 
> Then I'd propose to add compatible string for msm8916 too. We will need
> to distinguish between SoCs in order to select proper Venus firmware
> file too.
> 

This does make sense.

> 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.


With these patches you can compile the remoteproc driver builtin to the
kernel and once your system is booted you can issue:

  echo start > /sys/class/remoteproc/remoteproc0/state

and venus should boot and the codec should be probed.

  echo stop > /sys/class/remoteproc/remoteproc0/state

will shut down the core after removing the codec.


Please let me know if you think this is a viable solution.

(This is only tested on db410c so far, looks like we might need a few
more clocks for 8996 and we should probably be explicit about the noc
and its resources)

[1] https://git.linaro.org/people/stanimir.varbanov/linux.git/log/?h=release/qcomlt-4.9-venus-wip
[2] https://github.com/andersson/kernel/commits/qcomlt/v4.9-rproc-backport
[3] https://github.com/andersson/kernel/commits/qcomlt/venus-wip

Regards,
Bjorn