Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 Aug 2022 18:32:51 +0300
From:   Abel Vesa <abel.vesa@linaro.org>
To:     Caleb Connolly <caleb.connolly@linaro.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Maulik Shah <quic_mkshah@quicinc.com>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/4] arm64: dts: qcom: sdm845: Add the RPMh stats node
Message-ID: <Yvu4o1bFdKLfvaiL@linaro.org>
References: <20220812101240.1869605-1-abel.vesa@linaro.org>
 <T2Uz7zs4Ht58lYc5zWg1VBY0ju6bVaSKa9y3RhBQWDDHmPXBHbAxI2J34jSeY0BFQy2y4JtFn3nQS0Lz4xI5jw==@protonmail.internalid>
 <20220812101240.1869605-3-abel.vesa@linaro.org>
 <e6821eef-4fcb-97b1-24be-e2bb62b99039@linaro.org>
 <Yvtx2aK1Uu51hTPM@linaro.org>
 <b34b2fa6-7dbf-e4d3-9833-57efd42f9137@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b34b2fa6-7dbf-e4d3-9833-57efd42f9137@linaro.org>
Precedence: bulk

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/20220628201340.3981860-1-swboyd@chromium.org/
> > 
> > > > 
> > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> 
> Tested-by: Caleb Connolly <caleb.connolly@linaro.org>
> > > > ---
> > > > 
> > > > 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)