All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock
@ 2017-02-15  4:51 Jonathan Neuschäfer
       [not found] ` <20170215045157.11659-1-j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Neuschäfer @ 2017-02-15  4:51 UTC (permalink / raw)
  To: linux-arm-msm; +Cc: devicetree, Andy Gross, Jonathan Neuschäfer

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";
+
 		memory-region = <&adsp_region>;
 
 		qcom,smem-states = <&adsp_smp2p_out 0>;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers
       [not found] ` <20170215045157.11659-1-j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
@ 2017-02-15  4:51   ` Jonathan Neuschäfer
       [not found]     ` <20170215045157.11659-2-j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
  2017-02-20 22:00   ` [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock Andy Gross
  1 sibling, 1 reply; 11+ messages in thread
From: Jonathan Neuschäfer @ 2017-02-15  4:51 UTC (permalink / raw)
  To: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Andy Gross, Jonathan Neuschäfer

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
---
 arch/arm/configs/qcom_defconfig | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/configs/qcom_defconfig b/arch/arm/configs/qcom_defconfig
index 4ffdd607205d..97b9e9cd2292 100644
--- a/arch/arm/configs/qcom_defconfig
+++ b/arch/arm/configs/qcom_defconfig
@@ -190,11 +190,16 @@ CONFIG_MDM_LCC_9615=y
 CONFIG_MSM_MMCC_8960=y
 CONFIG_MSM_MMCC_8974=y
 CONFIG_HWSPINLOCK_QCOM=y
+CONFIG_REMOTEPROC=y
+CONFIG_QCOM_ADSP_PIL=y
+CONFIG_QCOM_Q6V5_PIL=y
+CONFIG_QCOM_WCNSS_PIL=y
 CONFIG_QCOM_GSBI=y
 CONFIG_QCOM_PM=y
 CONFIG_QCOM_SMEM=y
 CONFIG_QCOM_SMD=y
 CONFIG_QCOM_SMD_RPM=y
+CONFIG_QCOM_SMP2P=y
 CONFIG_IIO=y
 CONFIG_IIO_BUFFER_CB=y
 CONFIG_IIO_SW_TRIGGER=y
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock
       [not found] ` <20170215045157.11659-1-j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
  2017-02-15  4:51   ` [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers Jonathan Neuschäfer
@ 2017-02-20 22:00   ` Andy Gross
  2017-02-21  5:45     ` Bjorn Andersson
  1 sibling, 1 reply; 11+ messages in thread
From: Andy Gross @ 2017-02-20 22:00 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A

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-hi6Y0CQ0nG0@public.gmane.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
> ---
>  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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock
  2017-02-20 22:00   ` [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock Andy Gross
@ 2017-02-21  5:45     ` Bjorn Andersson
  2017-02-21 16:29       ` Jonathan Neuschäfer
  0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-02-21  5:45 UTC (permalink / raw)
  To: Andy Gross; +Cc: Jonathan Neusch?fer, linux-arm-msm, devicetree

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock
  2017-02-21  5:45     ` Bjorn Andersson
@ 2017-02-21 16:29       ` Jonathan Neuschäfer
  0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Neuschäfer @ 2017-02-21 16:29 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andy Gross, Jonathan Neusch?fer,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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-hi6Y0CQ0nG0@public.gmane.org>
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers
       [not found]     ` <20170215045157.11659-2-j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
@ 2017-03-04  1:20       ` Bjorn Andersson
  2017-03-05  5:01         ` Jonathan Neuschäfer
  0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-03-04  1:20 UTC (permalink / raw)
  To: Jonathan Neusch?fer
  Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Andy Gross

On Tue 14 Feb 20:51 PST 2017, Jonathan Neusch?fer wrote:

> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  arch/arm/configs/qcom_defconfig | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/configs/qcom_defconfig b/arch/arm/configs/qcom_defconfig
> index 4ffdd607205d..97b9e9cd2292 100644
> --- a/arch/arm/configs/qcom_defconfig
> +++ b/arch/arm/configs/qcom_defconfig
> @@ -190,11 +190,16 @@ CONFIG_MDM_LCC_9615=y
>  CONFIG_MSM_MMCC_8960=y
>  CONFIG_MSM_MMCC_8974=y
>  CONFIG_HWSPINLOCK_QCOM=y
> +CONFIG_REMOTEPROC=y
> +CONFIG_QCOM_ADSP_PIL=y
> +CONFIG_QCOM_Q6V5_PIL=y
> +CONFIG_QCOM_WCNSS_PIL=y
>  CONFIG_QCOM_GSBI=y
>  CONFIG_QCOM_PM=y
>  CONFIG_QCOM_SMEM=y
>  CONFIG_QCOM_SMD=y
>  CONFIG_QCOM_SMD_RPM=y
> +CONFIG_QCOM_SMP2P=y

We also need CONFIG_QCOM_SMSM=y here, its currently used to signal state
of the ring buffers for WiFi.

With the addition of that you have my:

Acked-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers
  2017-03-04  1:20       ` Bjorn Andersson
@ 2017-03-05  5:01         ` Jonathan Neuschäfer
  2017-03-05 16:49           ` Bjorn Andersson
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Neuschäfer @ 2017-03-05  5:01 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Jonathan Neusch?fer, linux-arm-msm, devicetree, Andy Gross

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Fri, Mar 03, 2017 at 05:20:32PM -0800, Bjorn Andersson wrote:
> On Tue 14 Feb 20:51 PST 2017, Jonathan Neusch?fer wrote:
> 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> >  arch/arm/configs/qcom_defconfig | 5 +++++
> >  1 file changed, 5 insertions(+)
[...]
> > +CONFIG_QCOM_SMP2P=y
> 
> We also need CONFIG_QCOM_SMSM=y here, its currently used to signal state
> of the ring buffers for WiFi.

FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone,
based on MSM8974; I'm using the Sony Xperia Honami DT because it's close
enough), and I think it failed to initialize:

[    0.647743] qcom-smsm smsm: no smsm size info, using defaults
[    0.647775] qcom-smsm smsm: unable to allocate shared state entry

I think CONFIG_QCOM_WCNSS_CTRL may be needed too, but I'll leave that
for a future patch because I don't understand WCNSS well enough.

> With the addition of that you have my:
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

I'll send a v2 of this series with your R-b and A-b tags.


Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers
  2017-03-05  5:01         ` Jonathan Neuschäfer
@ 2017-03-05 16:49           ` Bjorn Andersson
  2017-03-07  2:01             ` Jonathan Neuschäfer
  0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-03-05 16:49 UTC (permalink / raw)
  To: Jonathan Neusch?fer
  Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Andy Gross

On Sun 05 Mar 05:01 GMT 2017, Jonathan Neusch?fer wrote:

> On Fri, Mar 03, 2017 at 05:20:32PM -0800, Bjorn Andersson wrote:
> > On Tue 14 Feb 20:51 PST 2017, Jonathan Neusch?fer wrote:
> > 
> > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
> > > ---
> > >  arch/arm/configs/qcom_defconfig | 5 +++++
> > >  1 file changed, 5 insertions(+)
> [...]
> > > +CONFIG_QCOM_SMP2P=y
> > 
> > We also need CONFIG_QCOM_SMSM=y here, its currently used to signal state
> > of the ring buffers for WiFi.
> 
> FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone,
> based on MSM8974; I'm using the Sony Xperia Honami DT because it's close
> enough), and I think it failed to initialize:
> 

Using Honami should work so far, but please do write a patch adding the
Padfone, so that we don't accidentally break your HW at some point.

> [    0.647743] qcom-smsm smsm: no smsm size info, using defaults
> [    0.647775] qcom-smsm smsm: unable to allocate shared state entry
> 

Could you please confirm where in qcom_smem_alloc_global() we're
failing? As far as I can tell we should fail with -EEXIST or if the
passed "size" parameter is bogus -ENOMEM (but the default number of
entries really should be less than the amount of free SMEM space).

> I think CONFIG_QCOM_WCNSS_CTRL may be needed too, but I'll leave that
> for a future patch because I don't understand WCNSS well enough.
> 

Missed that one, when the WCNSS firmware boots the WCNSS_CTRL driver is
probed - it will upload the NV parameter file to the WCNSS "OS" and when
that is done it will probe the WiFi and BT drivers.

So, you need it as well.

> > With the addition of that you have my:
> > 
> > Acked-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> I'll send a v2 of this series with your R-b and A-b tags.
> 

Thanks,
Bjorn


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers
  2017-03-05 16:49           ` Bjorn Andersson
@ 2017-03-07  2:01             ` Jonathan Neuschäfer
  2017-03-07 11:04               ` Bjorn Andersson
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Neuschäfer @ 2017-03-07  2:01 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Jonathan Neusch?fer, linux-arm-msm, devicetree, Andy Gross

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

On Sun, Mar 05, 2017 at 04:49:30PM +0000, Bjorn Andersson wrote:
> On Sun 05 Mar 05:01 GMT 2017, Jonathan Neusch?fer wrote:
[...]
> > FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone,
> > based on MSM8974; I'm using the Sony Xperia Honami DT because it's close
> > enough), and I think it failed to initialize:
> > 
> 
> Using Honami should work so far, but please do write a patch adding the
> Padfone, so that we don't accidentally break your HW at some point.

I can try how far I get, but unfortunately I don't have hardware
documentation or schematics, because I'm a hobbyist.

> > [    0.647743] qcom-smsm smsm: no smsm size info, using defaults
> > [    0.647775] qcom-smsm smsm: unable to allocate shared state entry
> > 
> 
> Could you please confirm where in qcom_smem_alloc_global() we're
> failing? As far as I can tell we should fail with -EEXIST or if the
> passed "size" parameter is bogus -ENOMEM (but the default number of
> entries really should be less than the amount of free SMEM space).

* qcom_smem_get returns -EPROBE_DEFER:

	void *ptr = ERR_PTR(-EPROBE_DEFER);
	if (!__smem)
		return ptr;

* smsm_get_size_info prints "no smsm size info, using defaults\n"
* qcom_smem_alloc also returns -EPROBE_DEFER early.


BTW, I think smsm_get_size_info is using uninitialized memory here:

        size_t size;    /* is uninitialized */
        struct { ... } *info;

        /* qcom_smem_get returns early without setting size */
        info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size);

        /*
	 * PTR_ERR(info) is not -ENOENT.
         * size (still uninitialized) is compared with the size of the local
         * struct defined above.
	 */
        if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) {
                ...
        }

> > I think CONFIG_QCOM_WCNSS_CTRL may be needed too, but I'll leave that
> > for a future patch because I don't understand WCNSS well enough.
> > 
> 
> Missed that one, when the WCNSS firmware boots the WCNSS_CTRL driver is
> probed - it will upload the NV parameter file to the WCNSS "OS" and when
> that is done it will probe the WiFi and BT drivers.
> 
> So, you need it as well.

Ok, I'll add it to this patch.


Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers
  2017-03-07  2:01             ` Jonathan Neuschäfer
@ 2017-03-07 11:04               ` Bjorn Andersson
  2017-03-09  3:01                 ` Jonathan Neuschäfer
  0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-03-07 11:04 UTC (permalink / raw)
  To: Jonathan Neusch?fer; +Cc: linux-arm-msm, devicetree, Andy Gross

On Tue 07 Mar 03:01 CET 2017, Jonathan Neusch?fer wrote:

> On Sun, Mar 05, 2017 at 04:49:30PM +0000, Bjorn Andersson wrote:
> > On Sun 05 Mar 05:01 GMT 2017, Jonathan Neusch?fer wrote:
> [...]
> > > FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone,
> > > based on MSM8974; I'm using the Sony Xperia Honami DT because it's close
> > > enough), and I think it failed to initialize:
> > > 
> > 
> > Using Honami should work so far, but please do write a patch adding the
> > Padfone, so that we don't accidentally break your HW at some point.
> 
> I can try how far I get, but unfortunately I don't have hardware
> documentation or schematics, because I'm a hobbyist.
> 

If this is something that you will continue to hack on and you think
anyone else will be interested in having then please do submit patches
for it.

> > > [    0.647743] qcom-smsm smsm: no smsm size info, using defaults
> > > [    0.647775] qcom-smsm smsm: unable to allocate shared state entry
> > > 
> > 
> > Could you please confirm where in qcom_smem_alloc_global() we're
> > failing? As far as I can tell we should fail with -EEXIST or if the
> > passed "size" parameter is bogus -ENOMEM (but the default number of
> > entries really should be less than the amount of free SMEM space).
> 
> * qcom_smem_get returns -EPROBE_DEFER:
> 
> 	void *ptr = ERR_PTR(-EPROBE_DEFER);
> 	if (!__smem)
> 		return ptr;
> 
> * smsm_get_size_info prints "no smsm size info, using defaults\n"
> * qcom_smem_alloc also returns -EPROBE_DEFER early.
> 
> 
> BTW, I think smsm_get_size_info is using uninitialized memory here:
> 
>         size_t size;    /* is uninitialized */
>         struct { ... } *info;
> 
>         /* qcom_smem_get returns early without setting size */
>         info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size);
> 
>         /*
> 	 * PTR_ERR(info) is not -ENOENT.
>          * size (still uninitialized) is compared with the size of the local
>          * struct defined above.
> 	 */
>         if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) {
>                 ...
>         }
> 

Thanks for your analysis!

As you say the smsm driver is missing handling of probe deferral - which
is wrong. Could you please propose a patch checking for EPROBE_DEFER and
propagate this error as return value from probe()?  (Without an error
message)

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers
  2017-03-07 11:04               ` Bjorn Andersson
@ 2017-03-09  3:01                 ` Jonathan Neuschäfer
  0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Neuschäfer @ 2017-03-09  3:01 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Jonathan Neusch?fer, linux-arm-msm, devicetree, Andy Gross

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

On Tue, Mar 07, 2017 at 12:04:13PM +0100, Bjorn Andersson wrote:
> On Tue 07 Mar 03:01 CET 2017, Jonathan Neusch?fer wrote:
[...]
> If this is something that you will continue to hack on and you think
> anyone else will be interested in having then please do submit patches
> for it.

Ok, I'll give it a try.

> > > > [    0.647743] qcom-smsm smsm: no smsm size info, using defaults
> > > > [    0.647775] qcom-smsm smsm: unable to allocate shared state entry
> > > > 
> > > 
> > > Could you please confirm where in qcom_smem_alloc_global() we're
> > > failing? As far as I can tell we should fail with -EEXIST or if the
> > > passed "size" parameter is bogus -ENOMEM (but the default number of
> > > entries really should be less than the amount of free SMEM space).
> > 
> > * qcom_smem_get returns -EPROBE_DEFER:
> > 
> > 	void *ptr = ERR_PTR(-EPROBE_DEFER);
> > 	if (!__smem)
> > 		return ptr;
> > 
> > * smsm_get_size_info prints "no smsm size info, using defaults\n"
> > * qcom_smem_alloc also returns -EPROBE_DEFER early.
> > 
> > 
> > BTW, I think smsm_get_size_info is using uninitialized memory here:
> > 
> >         size_t size;    /* is uninitialized */
> >         struct { ... } *info;
> > 
> >         /* qcom_smem_get returns early without setting size */
> >         info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size);
> > 
> >         /*
> > 	 * PTR_ERR(info) is not -ENOENT.
> >          * size (still uninitialized) is compared with the size of the local
> >          * struct defined above.
> > 	 */
> >         if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) {
> >                 ...
> >         }
> > 
> 
> Thanks for your analysis!
> 
> As you say the smsm driver is missing handling of probe deferral - which
> is wrong. Could you please propose a patch checking for EPROBE_DEFER and
> propagate this error as return value from probe()?  (Without an error
> message)

I'm working on it.

Regarding the uninitialized memory read in "size != sizeof(*info)":
Is qcom_smem_get supposed to store a valid size value in any
non-probe-deferral case, or only in non-error cases?

Assuming the latter, I think "size != sizeof(*info)" should be changed
to "!IS_ERR(info) && size != sizeof(*info)", i.e. only check size if
qcom_smem_get returned success. Does that seem reasonable?



Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-03-09  3:09 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15  4:51 [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock Jonathan Neuschäfer
     [not found] ` <20170215045157.11659-1-j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
2017-02-15  4:51   ` [PATCH 2/2] ARM: qcom_defconfig: Enable Qualcomm remoteproc and SMP2P drivers Jonathan Neuschäfer
     [not found]     ` <20170215045157.11659-2-j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
2017-03-04  1:20       ` Bjorn Andersson
2017-03-05  5:01         ` Jonathan Neuschäfer
2017-03-05 16:49           ` Bjorn Andersson
2017-03-07  2:01             ` Jonathan Neuschäfer
2017-03-07 11:04               ` Bjorn Andersson
2017-03-09  3:01                 ` Jonathan Neuschäfer
2017-02-20 22:00   ` [PATCH 1/2] ARM: dts: msm8974: Hook up adsp-pil's xo clock Andy Gross
2017-02-21  5:45     ` Bjorn Andersson
2017-02-21 16:29       ` Jonathan Neuschäfer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.