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

[Abel Vesa <abel.vesa@linaro.org>]

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