| Message ID | 1480416647-3518-2-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 11/29, Avaneesh Kumar Dwivedi wrote: > This patch is based on > https://patchwork.kernel.org/patch/9415627/ > https://patchwork.kernel.org/patch/9415651/ > > This patch add clock initialization, enable and disable support. > Required resource name string and rating are differentiated based > on compatible string. Also added documentation for venus pil on > msm8996. > > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > --- > .../devicetree/bindings/remoteproc/qcom,venus.txt | 26 ++++- > drivers/remoteproc/qcom_venus_pil.c | 116 ++++++++++++++++++++- > 2 files changed, 140 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt > index 2d73ba1..417026b 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt > @@ -6,13 +6,30 @@ on the Qualcomm Venus remote processor core. > - compatible: > Usage: required > Value type: <string> > - Definition: must contain "qcom,venus-pil" > + Definition: must contain "qcom,venus-pil" or > + "qcom,venus-msm8996-pil" > > - memory-region: > Usage: required > Value type: <phandle> > Definition: a phandle to a node describing reserved memory > > +- clocks: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: reference to the core, iface and bus and maxi clocks to be held on > + behalf of the booting of the venus core > + > +- clock-names: > + Usage: required > + Value type: <stringlist> > + Definition: should be "core_clk", "iface_clk", "bus_clk", "maxi_clk" Please drop _clk from all clock names. > + > +- power-domains: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: reference to the venus gdsc to be turned on before booting venus core All these new properties can't be required if the original compatible is used, right? > + > * An example > reserved-memory { > #address-cells = <2>; > @@ -29,5 +46,12 @@ on the Qualcomm Venus remote processor core. > > rproc_venus@0 { > compatible = "qcom,venus-pil"; > + clocks = <&mmcc VIDEO_CORE_CLK>, > + <&mmcc VIDEO_AHB_CLK>, > + <&mmcc VIDEO_AXI_CLK>, > + <&mmcc VIDEO_MAXI_CLK>; > + > + clock-names = "core_clk", "iface_clk", "bus_clk", "maxi_clk"; > + power-domains = <&mmcc VENUS_GDSC>; > memory-region = <&venus_mem>; > };
On 11/30/2016 12:57 AM, Stephen Boyd wrote: > On 11/29, Avaneesh Kumar Dwivedi wrote: >> This patch is based on >> https://patchwork.kernel.org/patch/9415627/ >> https://patchwork.kernel.org/patch/9415651/ >> >> This patch add clock initialization, enable and disable support. >> Required resource name string and rating are differentiated based >> on compatible string. Also added documentation for venus pil on >> msm8996. >> >> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> >> --- >> .../devicetree/bindings/remoteproc/qcom,venus.txt | 26 ++++- >> drivers/remoteproc/qcom_venus_pil.c | 116 ++++++++++++++++++++- >> 2 files changed, 140 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >> index 2d73ba1..417026b 100644 >> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >> @@ -6,13 +6,30 @@ on the Qualcomm Venus remote processor core. >> - compatible: >> Usage: required >> Value type: <string> >> - Definition: must contain "qcom,venus-pil" >> + Definition: must contain "qcom,venus-pil" or >> + "qcom,venus-msm8996-pil" >> >> - memory-region: >> Usage: required >> Value type: <phandle> >> Definition: a phandle to a node describing reserved memory >> >> +- clocks: >> + Usage: required >> + Value type: <prop-encoded-array> >> + Definition: reference to the core, iface and bus and maxi clocks to be held on >> + behalf of the booting of the venus core >> + >> +- clock-names: >> + Usage: required >> + Value type: <stringlist> >> + Definition: should be "core_clk", "iface_clk", "bus_clk", "maxi_clk" > Please drop _clk from all clock names. OK > >> + >> +- power-domains: >> + Usage: required >> + Value type: <prop-encoded-array> >> + Definition: reference to the venus gdsc to be turned on before booting venus core > All these new properties can't be required if the original > compatible is used, right? gdsc were earlier handled via regulator framework, but now gdsc's are being handled via power domain framework. so any driver which require gdsc to be enabled need to use this property on mainline kernel. so these properties will even be applicable for 8916 venus rproc on mainline kernel. > >> + >> * An example >> reserved-memory { >> #address-cells = <2>; >> @@ -29,5 +46,12 @@ on the Qualcomm Venus remote processor core. >> >> rproc_venus@0 { >> compatible = "qcom,venus-pil"; >> + clocks = <&mmcc VIDEO_CORE_CLK>, >> + <&mmcc VIDEO_AHB_CLK>, >> + <&mmcc VIDEO_AXI_CLK>, >> + <&mmcc VIDEO_MAXI_CLK>; >> + >> + clock-names = "core_clk", "iface_clk", "bus_clk", "maxi_clk"; >> + power-domains = <&mmcc VENUS_GDSC>; >> memory-region = <&venus_mem>; >> };
Hi, On 11/30/2016 07:24 AM, Dwivedi, Avaneesh Kumar (avani) wrote: > > > On 11/30/2016 12:57 AM, Stephen Boyd wrote: >> On 11/29, Avaneesh Kumar Dwivedi wrote: >>> This patch is based on >>> https://patchwork.kernel.org/patch/9415627/ >>> https://patchwork.kernel.org/patch/9415651/ >>> >>> This patch add clock initialization, enable and disable support. >>> Required resource name string and rating are differentiated based >>> on compatible string. Also added documentation for venus pil on >>> msm8996. >>> >>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> >>> --- >>> .../devicetree/bindings/remoteproc/qcom,venus.txt | 26 ++++- >>> drivers/remoteproc/qcom_venus_pil.c | 116 >>> ++++++++++++++++++++- >>> 2 files changed, 140 insertions(+), 2 deletions(-) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>> b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>> index 2d73ba1..417026b 100644 >>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>> @@ -6,13 +6,30 @@ on the Qualcomm Venus remote processor core. >>> - compatible: >>> Usage: required >>> Value type: <string> >>> - Definition: must contain "qcom,venus-pil" >>> + Definition: must contain "qcom,venus-pil" or >>> + "qcom,venus-msm8996-pil" >>> - memory-region: >>> Usage: required >>> Value type: <phandle> >>> Definition: a phandle to a node describing reserved memory >>> +- clocks: >>> + Usage: required >>> + Value type: <prop-encoded-array> >>> + Definition: reference to the core, iface and bus and maxi clocks >>> to be held on >>> + behalf of the booting of the venus core >>> + >>> +- clock-names: >>> + Usage: required >>> + Value type: <stringlist> >>> + Definition: should be "core_clk", "iface_clk", "bus_clk", >>> "maxi_clk" >> Please drop _clk from all clock names. > OK >> >>> + >>> +- power-domains: >>> + Usage: required >>> + Value type: <prop-encoded-array> >>> + Definition: reference to the venus gdsc to be turned on before >>> booting venus core >> All these new properties can't be required if the original >> compatible is used, right? > gdsc were earlier handled via regulator framework, but now gdsc's are > being handled via power domain framework. > so any driver which require gdsc to be enabled need to use this property > on mainline kernel. > so these properties will even be applicable for 8916 venus rproc on > mainline kernel. 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. 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?
On 11/30/2016 2:52 PM, Stanimir Varbanov wrote: > Hi, > > On 11/30/2016 07:24 AM, Dwivedi, Avaneesh Kumar (avani) wrote: >> >> On 11/30/2016 12:57 AM, Stephen Boyd wrote: >>> On 11/29, Avaneesh Kumar Dwivedi wrote: >>>> This patch is based on >>>> https://patchwork.kernel.org/patch/9415627/ >>>> https://patchwork.kernel.org/patch/9415651/ >>>> >>>> This patch add clock initialization, enable and disable support. >>>> Required resource name string and rating are differentiated based >>>> on compatible string. Also added documentation for venus pil on >>>> msm8996. >>>> >>>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> >>>> --- >>>> .../devicetree/bindings/remoteproc/qcom,venus.txt | 26 ++++- >>>> drivers/remoteproc/qcom_venus_pil.c | 116 >>>> ++++++++++++++++++++- >>>> 2 files changed, 140 insertions(+), 2 deletions(-) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>> b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>> index 2d73ba1..417026b 100644 >>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>> @@ -6,13 +6,30 @@ on the Qualcomm Venus remote processor core. >>>> - compatible: >>>> Usage: required >>>> Value type: <string> >>>> - Definition: must contain "qcom,venus-pil" >>>> + Definition: must contain "qcom,venus-pil" or >>>> + "qcom,venus-msm8996-pil" >>>> - memory-region: >>>> Usage: required >>>> Value type: <phandle> >>>> Definition: a phandle to a node describing reserved memory >>>> +- clocks: >>>> + Usage: required >>>> + Value type: <prop-encoded-array> >>>> + Definition: reference to the core, iface and bus and maxi clocks >>>> to be held on >>>> + behalf of the booting of the venus core >>>> + >>>> +- clock-names: >>>> + Usage: required >>>> + Value type: <stringlist> >>>> + Definition: should be "core_clk", "iface_clk", "bus_clk", >>>> "maxi_clk" >>> Please drop _clk from all clock names. >> OK >>>> + >>>> +- power-domains: >>>> + Usage: required >>>> + Value type: <prop-encoded-array> >>>> + Definition: reference to the venus gdsc to be turned on before >>>> booting venus core >>> All these new properties can't be required if the original >>> compatible is used, right? >> gdsc were earlier handled via regulator framework, but now gdsc's are >> being handled via power domain framework. >> so any driver which require gdsc to be enabled need to use this property >> on mainline kernel. >> so these properties will even be applicable for 8916 venus rproc on >> mainline kernel. > 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. > > 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? > Shall I go ahead and add runtime PM API calls during shutdown/start call along with other changes to address other comments? i was waiting any comment from Sboyed and Bjorn.
On 12/05/2016 11:46 AM, Dwivedi, Avaneesh Kumar (avani) wrote: > > > On 11/30/2016 2:52 PM, Stanimir Varbanov wrote: >> Hi, >> >> On 11/30/2016 07:24 AM, Dwivedi, Avaneesh Kumar (avani) wrote: >>> >>> On 11/30/2016 12:57 AM, Stephen Boyd wrote: >>>> On 11/29, Avaneesh Kumar Dwivedi wrote: >>>>> This patch is based on >>>>> https://patchwork.kernel.org/patch/9415627/ >>>>> https://patchwork.kernel.org/patch/9415651/ >>>>> >>>>> This patch add clock initialization, enable and disable support. >>>>> Required resource name string and rating are differentiated based >>>>> on compatible string. Also added documentation for venus pil on >>>>> msm8996. >>>>> >>>>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> >>>>> --- >>>>> .../devicetree/bindings/remoteproc/qcom,venus.txt | 26 ++++- >>>>> drivers/remoteproc/qcom_venus_pil.c | 116 >>>>> ++++++++++++++++++++- >>>>> 2 files changed, 140 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>>> b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>>> index 2d73ba1..417026b 100644 >>>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt >>>>> @@ -6,13 +6,30 @@ on the Qualcomm Venus remote processor core. >>>>> - compatible: >>>>> Usage: required >>>>> Value type: <string> >>>>> - Definition: must contain "qcom,venus-pil" >>>>> + Definition: must contain "qcom,venus-pil" or >>>>> + "qcom,venus-msm8996-pil" >>>>> - memory-region: >>>>> Usage: required >>>>> Value type: <phandle> >>>>> Definition: a phandle to a node describing reserved memory >>>>> +- clocks: >>>>> + Usage: required >>>>> + Value type: <prop-encoded-array> >>>>> + Definition: reference to the core, iface and bus and maxi clocks >>>>> to be held on >>>>> + behalf of the booting of the venus core >>>>> + >>>>> +- clock-names: >>>>> + Usage: required >>>>> + Value type: <stringlist> >>>>> + Definition: should be "core_clk", "iface_clk", "bus_clk", >>>>> "maxi_clk" >>>> Please drop _clk from all clock names. >>> OK >>>>> + >>>>> +- power-domains: >>>>> + Usage: required >>>>> + Value type: <prop-encoded-array> >>>>> + Definition: reference to the venus gdsc to be turned on before >>>>> booting venus core >>>> All these new properties can't be required if the original >>>> compatible is used, right? >>> gdsc were earlier handled via regulator framework, but now gdsc's are >>> being handled via power domain framework. >>> so any driver which require gdsc to be enabled need to use this property >>> on mainline kernel. >>> so these properties will even be applicable for 8916 venus rproc on >>> mainline kernel. >> 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. >> >> 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? >> > Shall I go ahead and add runtime PM API calls during shutdown/start call > along with other changes to address other comments? i was waiting any > comment from Sboyed and Bjorn. > IMO runtime should be added in remoteproc core, so you can start with an RFC patch to see how is going.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/17/2016 12:41 PM, Bjorn Andersson wrote: > 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. Does It mean that, you are thinking to abolish venus rproc driver? and video driver will use mdt loader and scm interface to boot venus core as clients make call? effectively avoiding any requirement for separate DT node for venus rproc? > > Regards, > Bjorn
Hi Bjorn, On 12/17/2016 09:11 AM, Bjorn Andersson wrote: > 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. Does it make sense if we move mdt parser in drivers/firmware ?
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt index 2d73ba1..417026b 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt @@ -6,13 +6,30 @@ on the Qualcomm Venus remote processor core. - compatible: Usage: required Value type: <string> - Definition: must contain "qcom,venus-pil" + Definition: must contain "qcom,venus-pil" or + "qcom,venus-msm8996-pil" - memory-region: Usage: required Value type: <phandle> Definition: a phandle to a node describing reserved memory +- clocks: + Usage: required + Value type: <prop-encoded-array> + Definition: reference to the core, iface and bus and maxi clocks to be held on + behalf of the booting of the venus core + +- clock-names: + Usage: required + Value type: <stringlist> + Definition: should be "core_clk", "iface_clk", "bus_clk", "maxi_clk" + +- power-domains: + Usage: required + Value type: <prop-encoded-array> + Definition: reference to the venus gdsc to be turned on before booting venus core + * An example reserved-memory { #address-cells = <2>; @@ -29,5 +46,12 @@ on the Qualcomm Venus remote processor core. rproc_venus@0 { compatible = "qcom,venus-pil"; + clocks = <&mmcc VIDEO_CORE_CLK>, + <&mmcc VIDEO_AHB_CLK>, + <&mmcc VIDEO_AXI_CLK>, + <&mmcc VIDEO_MAXI_CLK>; + + clock-names = "core_clk", "iface_clk", "bus_clk", "maxi_clk"; + power-domains = <&mmcc VENUS_GDSC>; memory-region = <&venus_mem>; }; diff --git a/drivers/remoteproc/qcom_venus_pil.c b/drivers/remoteproc/qcom_venus_pil.c index 6d4e55b..f91f873 100644 --- a/drivers/remoteproc/qcom_venus_pil.c +++ b/drivers/remoteproc/qcom_venus_pil.c @@ -19,8 +19,10 @@ #include <linux/module.h> #include <linux/of_reserved_mem.h> #include <linux/platform_device.h> +#include <linux/clk.h> #include <linux/qcom_scm.h> #include <linux/remoteproc.h> +#include <linux/of_device.h> #include "qcom_mdt_loader.h" #include "remoteproc_internal.h" @@ -30,6 +32,11 @@ #define VENUS_PAS_ID 9 #define VENUS_FW_MEM_SIZE SZ_8M +struct venus_rproc_res { + char **venus_clks; + int venus_clk_rate[4]; +}; + struct qcom_venus { struct device *dev; struct rproc *rproc; @@ -37,6 +44,8 @@ struct qcom_venus { phys_addr_t mem_phys; void *mem_va; size_t mem_size; + struct clk *venus_clks[4]; + int clk_count; }; static int venus_load(struct rproc *rproc, const struct firmware *fw) @@ -78,11 +87,49 @@ static int venus_load(struct rproc *rproc, const struct firmware *fw) .load = venus_load, }; +static int qcom_venus_clk_enable(struct device *dev, struct clk **clks, + int clk_count) +{ + int rc = 0; + int i; + + for (i = 0; i < clk_count; i++) { + rc = clk_prepare_enable(clks[i]); + if (rc) { + dev_err(dev, "Clock enable failed\n"); + goto err; + } + } + + return 0; +err: + for (i--; i >= 0; i--) + clk_disable_unprepare(clks[i]); + + return rc; +} + +static void qcom_venus_clk_disable(struct qcom_venus *venus) +{ + int i; + struct clk **clks = venus->venus_clks; + + for (i = 0; i < venus->clk_count; i++) + clk_disable_unprepare(clks[i]); +} + static int venus_start(struct rproc *rproc) { struct qcom_venus *venus = rproc->priv; int ret; + ret = qcom_venus_clk_enable(venus->dev, venus->venus_clks, + venus->clk_count); + if (ret) { + dev_err(venus->dev, "failed to enable venus_clk\n"); + return ret; + } + ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID); if (ret) dev_err(venus->dev, @@ -101,6 +148,8 @@ static int venus_stop(struct rproc *rproc) if (ret) dev_err(venus->dev, "failed to shutdown: %d\n", ret); + qcom_venus_clk_disable(venus); + return ret; } @@ -123,13 +172,58 @@ static void *venus_da_to_va(struct rproc *rproc, u64 da, int len) .da_to_va = venus_da_to_va, }; +static int qcom_venus_init_clocks(struct device *dev, struct clk **clks, + char **clk_str, const int *rate) +{ + int clk_count = 0, i; + + if (!clk_str) + return 0; + + while (clk_str[clk_count] != NULL) + clk_count++; + + if (!clk_count) + return clk_count; + + if (!clks) + return -ENOMEM; + + for (i = 0; i < clk_count; i++) { + const char *clock_name; + + clock_name = clk_str[i]; + clks[i] = devm_clk_get(dev, clock_name); + if (IS_ERR(clks[i])) { + + int rc = PTR_ERR(clks[i]); + + if (rc != -EPROBE_DEFER) + dev_err(dev, "Failed to get %s clock\n", + clock_name); + return rc; + } + clk_set_rate(clks[i], clk_round_rate(clks[i], rate[i])); + + } + + return clk_count; +} + + + static int venus_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct qcom_venus *venus; struct rproc *rproc; + const struct venus_rproc_res *desc; int ret; + desc = of_device_get_match_data(&pdev->dev); + if (!desc) + return -EINVAL; + if (!qcom_scm_is_available()) return -EPROBE_DEFER; @@ -158,6 +252,14 @@ static int venus_probe(struct platform_device *pdev) platform_set_drvdata(pdev, venus); + ret = qcom_venus_init_clocks(&pdev->dev, venus->venus_clks, + desc->venus_clks, desc->venus_clk_rate); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to setup venus clocks.\n"); + return ret; + } + venus->clk_count = ret; + venus->mem_va = dma_alloc_coherent(dev, venus->mem_size, &venus->mem_phys, GFP_KERNEL); if (!venus->mem_va) { @@ -194,8 +296,20 @@ static int venus_remove(struct platform_device *pdev) return 0; } +static const struct venus_rproc_res venus_msm8996_res = { + .venus_clks = (char*[]){"core_clk", "iface_clk", "bus_clk", + "maxi_clk", NULL}, + .venus_clk_rate = {19200000, 19200000, 19200000, 80000000}, +}; + +static const struct venus_rproc_res venus_8916_res = { + .venus_clks = NULL, + .venus_clk_rate = {0}, +}; + static const struct of_device_id venus_of_match[] = { - { .compatible = "qcom,venus-pil" }, + { .compatible = "qcom,venus-msm8996-pil", .data = &venus_msm8996_res }, + { .compatible = "qcom,venus-pil", .data = &venus_8916_res}, { }, };
This patch is based on https://patchwork.kernel.org/patch/9415627/ https://patchwork.kernel.org/patch/9415651/ This patch add clock initialization, enable and disable support. Required resource name string and rating are differentiated based on compatible string. Also added documentation for venus pil on msm8996. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- .../devicetree/bindings/remoteproc/qcom,venus.txt | 26 ++++- drivers/remoteproc/qcom_venus_pil.c | 116 ++++++++++++++++++++- 2 files changed, 140 insertions(+), 2 deletions(-)