2022-08-12 10:23:50

by Abel Vesa

[permalink] [raw]
Subject: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node

SDM845 is a special case compared to the other platforms that use RPMh
stats, since it only has 2 stats (aosd and cxsd), while the others have
a 3rd one (ddr).

So lets add the node but with a SDM845 dedicated compatible to make
the driver aware of the different stats config.

Signed-off-by: Abel Vesa <[email protected]>
---

Changed qcom,rpmh-stats-sdm845 to qcom,sdm845-rpmh-stats, as suggested
by Krzysztof.

arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 5bea96a9ce06..67fe08b837be 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -4851,6 +4851,11 @@ ebi_cdev: ebi {
};
};

+ sram@c3f0000 {
+ compatible = "qcom,sdm845-rpmh-stats";
+ reg = <0 0x0c3f0000 0 0x400>;
+ };
+
spmi_bus: spmi@c440000 {
compatible = "qcom,spmi-pmic-arb";
reg = <0 0x0c440000 0 0x1100>,
--
2.34.1


2022-08-12 10:56:44

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node

On 12/08/2022 13:12, Abel Vesa wrote:
> SDM845 is a special case compared to the other platforms that use RPMh
> stats, since it only has 2 stats (aosd and cxsd), while the others have
> a 3rd one (ddr).
>
> So lets add the node but with a SDM845 dedicated compatible to make
> the driver aware of the different stats config.
>
> Signed-off-by: Abel Vesa <[email protected]>


Acked-by: Krzysztof Kozlowski <[email protected]>


Best regards,
Krzysztof

2022-08-16 00:37:46

by Caleb Connolly

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node



On 12/08/2022 11:12, Abel Vesa wrote:
> SDM845 is a special case compared to the other platforms that use RPMh
> stats, since it only has 2 stats (aosd and cxsd), while the others have
> a 3rd one (ddr).
>
> So lets add the node but with a SDM845 dedicated compatible to make
> the driver aware of the different stats config.
Hi,

I gave this a go on the OnePlus 6, I noticed the driver is also meant to read
the stats for remote procs via smem, however this seems to fail for me - it
can't find any of the SMEM items even if I probe the driver manually after
ensuring remoteprocs have booted. Is this an unsupported feature on SDM845?
>
> Signed-off-by: Abel Vesa <[email protected]>
> ---
>
> Changed qcom,rpmh-stats-sdm845 to qcom,sdm845-rpmh-stats, as suggested
> by Krzysztof.
>
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 5bea96a9ce06..67fe08b837be 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -4851,6 +4851,11 @@ ebi_cdev: ebi {
> };
> };
>
> + sram@c3f0000 {
> + compatible = "qcom,sdm845-rpmh-stats";
> + reg = <0 0x0c3f0000 0 0x400>;
> + };
> +
> spmi_bus: spmi@c440000 {
> compatible = "qcom,spmi-pmic-arb";
> reg = <0 0x0c440000 0 0x1100>,
> --
> 2.34.1
>

--
Kind Regards,
Caleb (they/he)

2022-08-16 11:45:54

by Abel Vesa

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node

On 22-08-15 21:34:07, Caleb Connolly wrote:
>
>
> On 12/08/2022 11:12, Abel Vesa wrote:
> > SDM845 is a special case compared to the other platforms that use RPMh
> > stats, since it only has 2 stats (aosd and cxsd), while the others have
> > a 3rd one (ddr).
> >
> > So lets add the node but with a SDM845 dedicated compatible to make
> > the driver aware of the different stats config.
> Hi,
>
> I gave this a go on the OnePlus 6, I noticed the driver is also meant to
> read the stats for remote procs via smem, however this seems to fail for me
> - it can't find any of the SMEM items even if I probe the driver manually
> after ensuring remoteprocs have booted. Is this an unsupported feature on
> SDM845?

Thanks for giving it a test.

Actually, you need to probe the qcom_stats after the remoteprocs have
booted.

Doing so, you'll end up having the following:
adsp aosd cdsp cxsd

> >
> > Signed-off-by: Abel Vesa <[email protected]>
> > ---
> >
> > Changed qcom,rpmh-stats-sdm845 to qcom,sdm845-rpmh-stats, as suggested
> > by Krzysztof.
> >
> > arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 5bea96a9ce06..67fe08b837be 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -4851,6 +4851,11 @@ ebi_cdev: ebi {
> > };
> > };
> >
> > + sram@c3f0000 {
> > + compatible = "qcom,sdm845-rpmh-stats";
> > + reg = <0 0x0c3f0000 0 0x400>;
> > + };
> > +
> > spmi_bus: spmi@c440000 {
> > compatible = "qcom,spmi-pmic-arb";
> > reg = <0 0x0c440000 0 0x1100>,
> > --
> > 2.34.1
> >
>
> --
> Kind Regards,
> Caleb (they/he)

2022-08-16 14:34:52

by Caleb Connolly

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node



On 16/08/2022 11:30, Abel Vesa wrote:
> On 22-08-15 21:34:07, Caleb Connolly wrote:
>>
>>
>> On 12/08/2022 11:12, Abel Vesa wrote:
>>> SDM845 is a special case compared to the other platforms that use RPMh
>>> stats, since it only has 2 stats (aosd and cxsd), while the others have
>>> a 3rd one (ddr).
>>>
>>> So lets add the node but with a SDM845 dedicated compatible to make
>>> the driver aware of the different stats config.
>> Hi,
>>
>> I gave this a go on the OnePlus 6, I noticed the driver is also meant to
>> read the stats for remote procs via smem, however this seems to fail for me
>> - it can't find any of the SMEM items even if I probe the driver manually
>> after ensuring remoteprocs have booted. Is this an unsupported feature on
>> SDM845?
>
> Thanks for giving it a test.
>
> Actually, you need to probe the qcom_stats after the remoteprocs have
> booted.

Hi, thanks for getting back to me. I did try this as mentioned above but I think I must have been
doing something wrong as I get different behaviour now:


enchilada:/ # cat /sys/class/remoteproc/remoteproc*/state

running

running

running

running

enchilada:/ # ls /d/qcom_stats/
aosd cxsd
enchilada:/ # rmmod qcom_stats
enchilada:/ # insmod /data/qcom_stats.ko
enchilada:/ # ls /d/qcom_stats/
adsp aosd cdsp cxsd modem slpi



Weirdly, despite it succeeding it prints the following in dmesg with logging added to
qcom_create_subsystem_stat_files() [1]:

[ 156.540307] Couldn't get smem object 'wpss' (item: 605, pid: 13): -2
[ 156.546899] Couldn't get smem object 'gpu' (item: 609, pid: 0): -2
[ 156.553260] Couldn't get smem object 'display' (item: 610, pid: 0): -2
[ 156.559957] Couldn't get smem object 'adsp_island' (item: 613, pid: 2): -2
[ 156.567007] Couldn't get smem object 'slpi_island' (item: 613, pid: 3): -2
>
> Doing so, you'll end up having the following:
> adsp aosd cdsp cxsd

I seem to get a few more, I have some out of tree patches enabling the SLPI, and iirc the db845c
doesn't have a full modem firmware. If these look good to you I'd appreciate it if you add my Tested-by.

enchilada:/ # for x in /d/qcom_stats/*; do echo $x; cat $x; done
/d/qcom_stats/adsp
Count: 48
Last Entered At: 1199663157
Last Exited At: 1524359015
Accumulated Duration: 793060082
/d/qcom_stats/aosd
Count: 0
Last Entered At: 0
Last Exited At: 0
Accumulated Duration: 0
/d/qcom_stats/cdsp
Count: 35
Last Entered At: 1194818037
Last Exited At: 1194769648
Accumulated Duration: 3223580811
/d/qcom_stats/cxsd
Count: 0
Last Entered At: 0
Last Exited At: 0
Accumulated Duration: 0
/d/qcom_stats/modem
Count: 49
Last Entered At: 3687081003
Last Exited At: 3686727026
Accumulated Duration: 2915592136
/d/qcom_stats/slpi
Count: 53
Last Entered At: 3120905905
Last Exited At: 3120894535
Accumulated Duration: 3218969498

Am I right in thinking the aosd and cxsd being all 0 is probably a similar issue to the one reported
by Stephen in [2]?


[1]:

diff --git a/drivers/soc/qcom/qcom_stats.c b/drivers/soc/qcom/qcom_stats.c
index 121ea409fafc..56cfb20d3683 100644
--- a/drivers/soc/qcom/qcom_stats.c
+++ b/drivers/soc/qcom/qcom_stats.c
@@ -178,8 +178,12 @@ static void qcom_create_subsystem_stat_files(struct dentry *root,

for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
- if (IS_ERR(stat))
+ if (IS_ERR(stat)) {
+ pr_info("Couldn't get smem object '%s' (item: %d, pid: %d): %ld\n",
+ subsystems[i].name, subsystems[i].smem_item, subsystems[i].pid,
+ PTR_ERR(stat));
continue;
+ }

debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
&qcom_subsystem_sleep_stats_fops);



[2]: https://lore.kernel.org/linux-arm-msm/[email protected]/
>
>>>
>>> Signed-off-by: Abel Vesa <[email protected]>

Tested-by: Caleb Connolly <[email protected]>
>>> ---
>>>
>>> Changed qcom,rpmh-stats-sdm845 to qcom,sdm845-rpmh-stats, as suggested
>>> by Krzysztof.
>>>
>>> arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> index 5bea96a9ce06..67fe08b837be 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> @@ -4851,6 +4851,11 @@ ebi_cdev: ebi {
>>> };
>>> };
>>>
>>> + sram@c3f0000 {
>>> + compatible = "qcom,sdm845-rpmh-stats";
>>> + reg = <0 0x0c3f0000 0 0x400>;
>>> + };
>>> +
>>> spmi_bus: spmi@c440000 {
>>> compatible = "qcom,spmi-pmic-arb";
>>> reg = <0 0x0c440000 0 0x1100>,
>>> --
>>> 2.34.1
>>>
>>
>> --
>> Kind Regards,
>> Caleb (they/he)

--
Kind Regards,
Caleb (they/he)

2022-08-16 15:37:37

by Abel Vesa

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node

On 22-08-16 14:50:50, Caleb Connolly wrote:
>
>
> On 16/08/2022 11:30, Abel Vesa wrote:
> > On 22-08-15 21:34:07, Caleb Connolly wrote:
> > >
> > >
> > > On 12/08/2022 11:12, Abel Vesa wrote:
> > > > SDM845 is a special case compared to the other platforms that use RPMh
> > > > stats, since it only has 2 stats (aosd and cxsd), while the others have
> > > > a 3rd one (ddr).
> > > >
> > > > So lets add the node but with a SDM845 dedicated compatible to make
> > > > the driver aware of the different stats config.
> > > Hi,
> > >
> > > I gave this a go on the OnePlus 6, I noticed the driver is also meant to
> > > read the stats for remote procs via smem, however this seems to fail for me
> > > - it can't find any of the SMEM items even if I probe the driver manually
> > > after ensuring remoteprocs have booted. Is this an unsupported feature on
> > > SDM845?
> >
> > Thanks for giving it a test.
> >
> > Actually, you need to probe the qcom_stats after the remoteprocs have
> > booted.
>
> Hi, thanks for getting back to me. I did try this as mentioned above but I
> think I must have been doing something wrong as I get different behaviour
> now:
>
>
> enchilada:/ # cat /sys/class/remoteproc/remoteproc*/state
>
> running
>
> running
>
> running
>
> running
>
> enchilada:/ # ls /d/qcom_stats/
> aosd cxsd
> enchilada:/ # rmmod qcom_stats
> enchilada:/ # insmod /data/qcom_stats.ko
> enchilada:/ # ls /d/qcom_stats/
> adsp aosd cdsp cxsd modem slpi

Well, I run on upstream MTP, which has less enabled in devicetree.

>
>
>
> Weirdly, despite it succeeding it prints the following in dmesg with logging
> added to qcom_create_subsystem_stat_files() [1]:
>
> [ 156.540307] Couldn't get smem object 'wpss' (item: 605, pid: 13): -2
> [ 156.546899] Couldn't get smem object 'gpu' (item: 609, pid: 0): -2
> [ 156.553260] Couldn't get smem object 'display' (item: 610, pid: 0): -2
> [ 156.559957] Couldn't get smem object 'adsp_island' (item: 613, pid: 2): -2
> [ 156.567007] Couldn't get smem object 'slpi_island' (item: 613, pid: 3): -2

See my comment below your related changes.

> >
> > Doing so, you'll end up having the following:
> > adsp aosd cdsp cxsd
>
> I seem to get a few more, I have some out of tree patches enabling the SLPI,
> and iirc the db845c doesn't have a full modem firmware. If these look good
> to you I'd appreciate it if you add my Tested-by.

Looks OK to me.

>
> enchilada:/ # for x in /d/qcom_stats/*; do echo $x; cat $x; done
> /d/qcom_stats/adsp
> Count: 48
> Last Entered At: 1199663157
> Last Exited At: 1524359015
> Accumulated Duration: 793060082
> /d/qcom_stats/aosd
> Count: 0
> Last Entered At: 0
> Last Exited At: 0
> Accumulated Duration: 0
> /d/qcom_stats/cdsp
> Count: 35
> Last Entered At: 1194818037
> Last Exited At: 1194769648
> Accumulated Duration: 3223580811
> /d/qcom_stats/cxsd
> Count: 0
> Last Entered At: 0
> Last Exited At: 0
> Accumulated Duration: 0
> /d/qcom_stats/modem
> Count: 49
> Last Entered At: 3687081003
> Last Exited At: 3686727026
> Accumulated Duration: 2915592136
> /d/qcom_stats/slpi
> Count: 53
> Last Entered At: 3120905905
> Last Exited At: 3120894535
> Accumulated Duration: 3218969498
>
> Am I right in thinking the aosd and cxsd being all 0 is probably a similar
> issue to the one reported by Stephen in [2]?

Might be, I'm not sure. I'll have closer a look.

>
>
> [1]:
>
> diff --git a/drivers/soc/qcom/qcom_stats.c b/drivers/soc/qcom/qcom_stats.c
> index 121ea409fafc..56cfb20d3683 100644
> --- a/drivers/soc/qcom/qcom_stats.c
> +++ b/drivers/soc/qcom/qcom_stats.c
> @@ -178,8 +178,12 @@ static void qcom_create_subsystem_stat_files(struct dentry *root,
>
> for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
> - if (IS_ERR(stat))

Basically, the error here means the subsystem is not available right at
this moment. We could probably return EPROBE_DEFER here, but it really
depends on each platform's devicetree what remoteprocs they have
enabled or not. So I guess the safest thing to to is to just skip
quietly the ones that haven't probed yet.

> + if (IS_ERR(stat)) {
> + pr_info("Couldn't get smem object '%s' (item: %d, pid: %d): %ld\n",
> + subsystems[i].name, subsystems[i].smem_item, subsystems[i].pid,
> + PTR_ERR(stat));
> continue;
> + }
>
> debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
> &qcom_subsystem_sleep_stats_fops);
>
>
>
> [2]: https://lore.kernel.org/linux-arm-msm/[email protected]/
> >
> > > >
> > > > Signed-off-by: Abel Vesa <[email protected]>
>
> Tested-by: Caleb Connolly <[email protected]>
> > > > ---
> > > >
> > > > Changed qcom,rpmh-stats-sdm845 to qcom,sdm845-rpmh-stats, as suggested
> > > > by Krzysztof.
> > > >
> > > > arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > index 5bea96a9ce06..67fe08b837be 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > @@ -4851,6 +4851,11 @@ ebi_cdev: ebi {
> > > > };
> > > > };
> > > >
> > > > + sram@c3f0000 {
> > > > + compatible = "qcom,sdm845-rpmh-stats";
> > > > + reg = <0 0x0c3f0000 0 0x400>;
> > > > + };
> > > > +
> > > > spmi_bus: spmi@c440000 {
> > > > compatible = "qcom,spmi-pmic-arb";
> > > > reg = <0 0x0c440000 0 0x1100>,
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > --
> > > Kind Regards,
> > > Caleb (they/he)
>
> --
> Kind Regards,
> Caleb (they/he)

2022-08-18 02:08:39

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node

On Tue 16 Aug 10:32 CDT 2022, Abel Vesa wrote:

> On 22-08-16 14:50:50, Caleb Connolly wrote:
> >
> >
> > On 16/08/2022 11:30, Abel Vesa wrote:
> > > On 22-08-15 21:34:07, Caleb Connolly wrote:
> > > >
> > > >
> > > > On 12/08/2022 11:12, Abel Vesa wrote:
> > > > > SDM845 is a special case compared to the other platforms that use RPMh
> > > > > stats, since it only has 2 stats (aosd and cxsd), while the others have
> > > > > a 3rd one (ddr).
> > > > >
> > > > > So lets add the node but with a SDM845 dedicated compatible to make
> > > > > the driver aware of the different stats config.
> > > > Hi,
> > > >
> > > > I gave this a go on the OnePlus 6, I noticed the driver is also meant to
> > > > read the stats for remote procs via smem, however this seems to fail for me
> > > > - it can't find any of the SMEM items even if I probe the driver manually
> > > > after ensuring remoteprocs have booted. Is this an unsupported feature on
> > > > SDM845?
> > >
> > > Thanks for giving it a test.
> > >
> > > Actually, you need to probe the qcom_stats after the remoteprocs have
> > > booted.
> >
> > Hi, thanks for getting back to me. I did try this as mentioned above but I
> > think I must have been doing something wrong as I get different behaviour
> > now:
> >
> >
> > enchilada:/ # cat /sys/class/remoteproc/remoteproc*/state
> >
> > running
> >
> > running
> >
> > running
> >
> > running
> >
> > enchilada:/ # ls /d/qcom_stats/
> > aosd cxsd
> > enchilada:/ # rmmod qcom_stats
> > enchilada:/ # insmod /data/qcom_stats.ko
> > enchilada:/ # ls /d/qcom_stats/
> > adsp aosd cdsp cxsd modem slpi
>
> Well, I run on upstream MTP, which has less enabled in devicetree.
>
> >
> >
> >
> > Weirdly, despite it succeeding it prints the following in dmesg with logging
> > added to qcom_create_subsystem_stat_files() [1]:
> >
> > [ 156.540307] Couldn't get smem object 'wpss' (item: 605, pid: 13): -2
> > [ 156.546899] Couldn't get smem object 'gpu' (item: 609, pid: 0): -2
> > [ 156.553260] Couldn't get smem object 'display' (item: 610, pid: 0): -2
> > [ 156.559957] Couldn't get smem object 'adsp_island' (item: 613, pid: 2): -2
> > [ 156.567007] Couldn't get smem object 'slpi_island' (item: 613, pid: 3): -2
>
> See my comment below your related changes.
>
> > >
> > > Doing so, you'll end up having the following:
> > > adsp aosd cdsp cxsd
> >
> > I seem to get a few more, I have some out of tree patches enabling the SLPI,
> > and iirc the db845c doesn't have a full modem firmware. If these look good
> > to you I'd appreciate it if you add my Tested-by.
>
> Looks OK to me.
>
> >
> > enchilada:/ # for x in /d/qcom_stats/*; do echo $x; cat $x; done
> > /d/qcom_stats/adsp
> > Count: 48
> > Last Entered At: 1199663157
> > Last Exited At: 1524359015
> > Accumulated Duration: 793060082
> > /d/qcom_stats/aosd
> > Count: 0
> > Last Entered At: 0
> > Last Exited At: 0
> > Accumulated Duration: 0
> > /d/qcom_stats/cdsp
> > Count: 35
> > Last Entered At: 1194818037
> > Last Exited At: 1194769648
> > Accumulated Duration: 3223580811
> > /d/qcom_stats/cxsd
> > Count: 0
> > Last Entered At: 0
> > Last Exited At: 0
> > Accumulated Duration: 0
> > /d/qcom_stats/modem
> > Count: 49
> > Last Entered At: 3687081003
> > Last Exited At: 3686727026
> > Accumulated Duration: 2915592136
> > /d/qcom_stats/slpi
> > Count: 53
> > Last Entered At: 3120905905
> > Last Exited At: 3120894535
> > Accumulated Duration: 3218969498
> >
> > Am I right in thinking the aosd and cxsd being all 0 is probably a similar
> > issue to the one reported by Stephen in [2]?
>
> Might be, I'm not sure. I'll have closer a look.
>
> >
> >
> > [1]:
> >
> > diff --git a/drivers/soc/qcom/qcom_stats.c b/drivers/soc/qcom/qcom_stats.c
> > index 121ea409fafc..56cfb20d3683 100644
> > --- a/drivers/soc/qcom/qcom_stats.c
> > +++ b/drivers/soc/qcom/qcom_stats.c
> > @@ -178,8 +178,12 @@ static void qcom_create_subsystem_stat_files(struct dentry *root,
> >
> > for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> > stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
> > - if (IS_ERR(stat))
>
> Basically, the error here means the subsystem is not available right at
> this moment. We could probably return EPROBE_DEFER here, but it really
> depends on each platform's devicetree what remoteprocs they have
> enabled or not. So I guess the safest thing to to is to just skip
> quietly the ones that haven't probed yet.
>

Not only does the/each remoteproc driver need to probe, we actually need
to wait for them to boot in order to register their parts.

Perhaps we can use qcom_register_ssr_notifier() to dynamically register
these at some later time. This does however seem like a reasonable thing
to improve upon in a follow up series.

Regards,
Bjorn

2022-08-18 06:10:56

by Abel Vesa

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node

On 22-08-17 20:47:23, Bjorn Andersson wrote:
> On Tue 16 Aug 10:32 CDT 2022, Abel Vesa wrote:
>
> > On 22-08-16 14:50:50, Caleb Connolly wrote:
> > >
> > >
> > > On 16/08/2022 11:30, Abel Vesa wrote:
> > > > On 22-08-15 21:34:07, Caleb Connolly wrote:
> > > > >
> > > > >
> > > > > On 12/08/2022 11:12, Abel Vesa wrote:
> > > > > > SDM845 is a special case compared to the other platforms that use RPMh
> > > > > > stats, since it only has 2 stats (aosd and cxsd), while the others have
> > > > > > a 3rd one (ddr).
> > > > > >
> > > > > > So lets add the node but with a SDM845 dedicated compatible to make
> > > > > > the driver aware of the different stats config.
> > > > > Hi,
> > > > >
> > > > > I gave this a go on the OnePlus 6, I noticed the driver is also meant to
> > > > > read the stats for remote procs via smem, however this seems to fail for me
> > > > > - it can't find any of the SMEM items even if I probe the driver manually
> > > > > after ensuring remoteprocs have booted. Is this an unsupported feature on
> > > > > SDM845?
> > > >
> > > > Thanks for giving it a test.
> > > >
> > > > Actually, you need to probe the qcom_stats after the remoteprocs have
> > > > booted.
> > >
> > > Hi, thanks for getting back to me. I did try this as mentioned above but I
> > > think I must have been doing something wrong as I get different behaviour
> > > now:
> > >
> > >
> > > enchilada:/ # cat /sys/class/remoteproc/remoteproc*/state
> > >
> > > running
> > >
> > > running
> > >
> > > running
> > >
> > > running
> > >
> > > enchilada:/ # ls /d/qcom_stats/
> > > aosd cxsd
> > > enchilada:/ # rmmod qcom_stats
> > > enchilada:/ # insmod /data/qcom_stats.ko
> > > enchilada:/ # ls /d/qcom_stats/
> > > adsp aosd cdsp cxsd modem slpi
> >
> > Well, I run on upstream MTP, which has less enabled in devicetree.
> >
> > >
> > >
> > >
> > > Weirdly, despite it succeeding it prints the following in dmesg with logging
> > > added to qcom_create_subsystem_stat_files() [1]:
> > >
> > > [ 156.540307] Couldn't get smem object 'wpss' (item: 605, pid: 13): -2
> > > [ 156.546899] Couldn't get smem object 'gpu' (item: 609, pid: 0): -2
> > > [ 156.553260] Couldn't get smem object 'display' (item: 610, pid: 0): -2
> > > [ 156.559957] Couldn't get smem object 'adsp_island' (item: 613, pid: 2): -2
> > > [ 156.567007] Couldn't get smem object 'slpi_island' (item: 613, pid: 3): -2
> >
> > See my comment below your related changes.
> >
> > > >
> > > > Doing so, you'll end up having the following:
> > > > adsp aosd cdsp cxsd
> > >
> > > I seem to get a few more, I have some out of tree patches enabling the SLPI,
> > > and iirc the db845c doesn't have a full modem firmware. If these look good
> > > to you I'd appreciate it if you add my Tested-by.
> >
> > Looks OK to me.
> >
> > >
> > > enchilada:/ # for x in /d/qcom_stats/*; do echo $x; cat $x; done
> > > /d/qcom_stats/adsp
> > > Count: 48
> > > Last Entered At: 1199663157
> > > Last Exited At: 1524359015
> > > Accumulated Duration: 793060082
> > > /d/qcom_stats/aosd
> > > Count: 0
> > > Last Entered At: 0
> > > Last Exited At: 0
> > > Accumulated Duration: 0
> > > /d/qcom_stats/cdsp
> > > Count: 35
> > > Last Entered At: 1194818037
> > > Last Exited At: 1194769648
> > > Accumulated Duration: 3223580811
> > > /d/qcom_stats/cxsd
> > > Count: 0
> > > Last Entered At: 0
> > > Last Exited At: 0
> > > Accumulated Duration: 0
> > > /d/qcom_stats/modem
> > > Count: 49
> > > Last Entered At: 3687081003
> > > Last Exited At: 3686727026
> > > Accumulated Duration: 2915592136
> > > /d/qcom_stats/slpi
> > > Count: 53
> > > Last Entered At: 3120905905
> > > Last Exited At: 3120894535
> > > Accumulated Duration: 3218969498
> > >
> > > Am I right in thinking the aosd and cxsd being all 0 is probably a similar
> > > issue to the one reported by Stephen in [2]?
> >
> > Might be, I'm not sure. I'll have closer a look.
> >
> > >
> > >
> > > [1]:
> > >
> > > diff --git a/drivers/soc/qcom/qcom_stats.c b/drivers/soc/qcom/qcom_stats.c
> > > index 121ea409fafc..56cfb20d3683 100644
> > > --- a/drivers/soc/qcom/qcom_stats.c
> > > +++ b/drivers/soc/qcom/qcom_stats.c
> > > @@ -178,8 +178,12 @@ static void qcom_create_subsystem_stat_files(struct dentry *root,
> > >
> > > for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> > > stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
> > > - if (IS_ERR(stat))
> >
> > Basically, the error here means the subsystem is not available right at
> > this moment. We could probably return EPROBE_DEFER here, but it really
> > depends on each platform's devicetree what remoteprocs they have
> > enabled or not. So I guess the safest thing to to is to just skip
> > quietly the ones that haven't probed yet.
> >
>
> Not only does the/each remoteproc driver need to probe, we actually need
> to wait for them to boot in order to register their parts.
>
> Perhaps we can use qcom_register_ssr_notifier() to dynamically register
> these at some later time. This does however seem like a reasonable thing
> to improve upon in a follow up series.

Sounds like a good idea. Will give it a try and sent a patch a soon as I
have something working.

Thanks,
Abel

>
> Regards,
> Bjorn