diff mbox

[1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock

Message ID 20170215045157.11659-1-j.neuschaefer@gmx.net (mailing list archive)
State Superseded, archived
Delegated to: Andy Gross
Headers show

Commit Message

J. Neuschäfer Feb. 15, 2017, 4:51 a.m. UTC
Without this patch (and with CONFIG_QCOM_ADSP_PIL), I get this error:

	[    0.711529] qcom_adsp_pil adsp-pil: failed to get xo clock
	[    0.711540] remoteproc remoteproc0: releasing adsp-pil

With this patch, adsp-pil can initialize correctly.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

---
NOTE: I don't know if I actually chose the right clock source.
      Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
      suggests that adsp-pil's xo should be controlled by the RPM
      processor; Existing devicetrees and a recent patch to msm8996.dtsi
      use &xo_board, though:
      https://www.spinics.net/lists/linux-arm-msm/msg25711.html
---
 arch/arm/boot/dts/qcom-msm8974.dtsi | 3 +++
 1 file changed, 3 insertions(+)

Comments

Andy Gross Feb. 20, 2017, 10 p.m. UTC | #1
On Wed, Feb 15, 2017 at 05:51:56AM +0100, Jonathan Neuschäfer wrote:
> Without this patch (and with CONFIG_QCOM_ADSP_PIL), I get this error:
> 
> 	[    0.711529] qcom_adsp_pil adsp-pil: failed to get xo clock
> 	[    0.711540] remoteproc remoteproc0: releasing adsp-pil
> 
> With this patch, adsp-pil can initialize correctly.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> 
> ---
> NOTE: I don't know if I actually chose the right clock source.
>       Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
>       suggests that adsp-pil's xo should be controlled by the RPM
>       processor; Existing devicetrees and a recent patch to msm8996.dtsi
>       use &xo_board, though:
>       https://www.spinics.net/lists/linux-arm-msm/msg25711.html
> ---
>  arch/arm/boot/dts/qcom-msm8974.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> index d3e1a61b8671..cccd8cba8872 100644
> --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> @@ -250,6 +250,9 @@
>  
>  		cx-supply = <&pm8841_s2>;
>  
> +		clocks = <&xo_board>;
> +		clock-names = "xo";
> +

I have to wonder if this wasn't done specifically so that the board specific DTS
file would specify the XO clock.  If you do it here, it applies to all boards
using msm8974.

Maybe someone can answer that question.  Adding Bjorn to the CC.

Regards,

Andy
--
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
Bjorn Andersson Feb. 21, 2017, 5:45 a.m. UTC | #2
On Mon 20 Feb 14:00 PST 2017, Andy Gross wrote:

> On Wed, Feb 15, 2017 at 05:51:56AM +0100, Jonathan Neuschäfer wrote:
> > Without this patch (and with CONFIG_QCOM_ADSP_PIL), I get this error:
> > 
> > 	[    0.711529] qcom_adsp_pil adsp-pil: failed to get xo clock
> > 	[    0.711540] remoteproc remoteproc0: releasing adsp-pil
> > 
> > With this patch, adsp-pil can initialize correctly.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> > 
> > ---
> > NOTE: I don't know if I actually chose the right clock source.
> >       Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
> >       suggests that adsp-pil's xo should be controlled by the RPM
> >       processor; Existing devicetrees and a recent patch to msm8996.dtsi
> >       use &xo_board, though:
> >       https://www.spinics.net/lists/linux-arm-msm/msg25711.html

The ADSP is running off a clock derived from XO, as such a vote for XO
must be held while the ADSP is running. It's possible the all
application CPUs hit idle after launching the ADSP, but before the ADSP
firmware can communicate this need with the RPM, which would result in
the votes hitting 0 and the RPM might gate the XO.

To prevent this the ADSP remoteproc driver must hold an extra vote with
the RPM until the ADSP signals that it has cast its vote.

Unfortunately there are some issues with nested probe deferral in the
clock framework, so we can't accurately describe the XO today and as
we're still missing some last pieces of idle mechanism causing above
events we can survive by faking it and using xo_board - for a little
bit longer...

> > ---
> >  arch/arm/boot/dts/qcom-msm8974.dtsi | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> > index d3e1a61b8671..cccd8cba8872 100644
> > --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> > +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> > @@ -250,6 +250,9 @@
> >  
> >  		cx-supply = <&pm8841_s2>;
> >  
> > +		clocks = <&xo_board>;
> > +		clock-names = "xo";
> > +
> 
> I have to wonder if this wasn't done specifically so that the board specific DTS
> file would specify the XO clock.  If you do it here, it applies to all boards
> using msm8974.
> 

The reason for the adsp node missing the xo reference is that it
wasn't required originally. It is now and I didn't feel we had wide
spread usage and omitted backwards compatibility in the driver

> Maybe someone can answer that question.  Adding Bjorn to the CC.
> 

I forgot that I had sent you the original patch and hence didn't send
you a fix for it. Sorry about that, perhaps we should flag this for
stable?

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
J. Neuschäfer Feb. 21, 2017, 4:29 p.m. UTC | #3
On Mon, Feb 20, 2017 at 09:45:04PM -0800, Bjorn Andersson wrote:
> On Mon 20 Feb 14:00 PST 2017, Andy Gross wrote:
> 
> > On Wed, Feb 15, 2017 at 05:51:56AM +0100, Jonathan Neuschäfer wrote:
> > > Without this patch (and with CONFIG_QCOM_ADSP_PIL), I get this error:
> > > 
> > > 	[    0.711529] qcom_adsp_pil adsp-pil: failed to get xo clock
> > > 	[    0.711540] remoteproc remoteproc0: releasing adsp-pil
> > > 
> > > With this patch, adsp-pil can initialize correctly.
> > > 
> > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Thanks!

> The ADSP is running off a clock derived from XO, as such a vote for XO
> must be held while the ADSP is running. It's possible the all
> application CPUs hit idle after launching the ADSP, but before the ADSP
> firmware can communicate this need with the RPM, which would result in
> the votes hitting 0 and the RPM might gate the XO.
> 
> To prevent this the ADSP remoteproc driver must hold an extra vote with
> the RPM until the ADSP signals that it has cast its vote.
> 
> Unfortunately there are some issues with nested probe deferral in the
> clock framework, so we can't accurately describe the XO today and as
> we're still missing some last pieces of idle mechanism causing above
> events we can survive by faking it and using xo_board - for a little
> bit longer...

Ok, that makes sense.


Jonathan
--
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
diff mbox

Patch

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index d3e1a61b8671..cccd8cba8872 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -250,6 +250,9 @@ 
 
 		cx-supply = <&pm8841_s2>;
 
+		clocks = <&xo_board>;
+		clock-names = "xo";
+
 		memory-region = <&adsp_region>;
 
 		qcom,smem-states = <&adsp_smp2p_out 0>;