diff mbox series

[1/4] dt-bindings: soc: qcom: stats: add compatible for SM8150 platform

Message ID 20231209215601.3543895-1-dmitry.baryshkov@linaro.org
State New
Headers show
Series [1/4] dt-bindings: soc: qcom: stats: add compatible for SM8150 platform | expand

Commit Message

Dmitry Baryshkov Dec. 9, 2023, 9:55 p.m. UTC
The RPMh stats on the SM8150 platform have 3 stat slots (unlike sdm845),
but still doesn't have DDR sleep stats (unlike other generic platforms).
Add SoC-specific compatible string.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml | 1 +
 1 file changed, 1 insertion(+)

Comments

Konrad Dybcio Dec. 11, 2023, 9:11 a.m. UTC | #1
On 9.12.2023 22:56, Dmitry Baryshkov wrote:
> On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
> which demands platform-specific compatible and data.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
I don't think it makes sense considering the driver could detect the
presence (or possibility of presence) of DDR stats at runtime.

Konrad
Dmitry Baryshkov Dec. 11, 2023, 9:43 a.m. UTC | #2
On Mon, 11 Dec 2023 at 11:11, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 9.12.2023 22:56, Dmitry Baryshkov wrote:
> > On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
> > which demands platform-specific compatible and data.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> I don't think it makes sense considering the driver could detect the
> presence (or possibility of presence) of DDR stats at runtime.

No, it can not really. We have safety nets for checking the offset
value and then checking the magic number. But I'd prefer to be
explicit here. It's not that the 'invalid' data at this offset is 0 or
~0.
So, I'd prefer to be explicit here.

Actually we probably should have used SoC compat entries from the
beginning, even if looks ridiculous for such small thing.
Dmitry Baryshkov Dec. 11, 2023, 11:09 a.m. UTC | #3
On Mon, 11 Dec 2023 at 12:46, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 11.12.2023 10:43, Dmitry Baryshkov wrote:
> > On Mon, 11 Dec 2023 at 11:11, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 9.12.2023 22:56, Dmitry Baryshkov wrote:
> >>> On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
> >>> which demands platform-specific compatible and data.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >> I don't think it makes sense considering the driver could detect the
> >> presence (or possibility of presence) of DDR stats at runtime.
> >
> > No, it can not really. We have safety nets for checking the offset
> > value and then checking the magic number. But I'd prefer to be
> > explicit here. It's not that the 'invalid' data at this offset is 0 or
> > ~0.
> > So, I'd prefer to be explicit here.
> I'd say we're quite covered:
>
> if (ddr_stats_offset)
>         if (offset is within the range) // your latest patchset
>                 if (ddr_stats_magic)
>                         if (entries)
>                                 "show stats"
>                         else
>                                 "show nothing"
>                 else
>                         "no ddr stats"
>         else
>                 "no ddr stats"
> else
>         "no ddr stats"
>
> Konrad

I'd say, too many ifs. I'd prefer to have it disabled for this platform.
Doug Anderson Dec. 14, 2023, 12:59 a.m. UTC | #4
Hi,

On Mon, Dec 11, 2023 at 1:11 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 9.12.2023 22:55, Dmitry Baryshkov wrote:
> > The stats ram on sm8150 platform contains invalid data at the
> > DDR_DYNAMIC_OFFSET. Most likely this is because the platform didn't
> > support DDR sleep stats.
> Interesting. Can you read back DDR_DYNAMIC_OFFSET on 8350/8280 and
> see if 8150 has correct data in there?
>
> > However this platform uses generic
> > "qcom,rpmh-stats" compatible, which implies presense of the DDR data.
> > Add safety net to prevent old DTB files from crashing the
> > qcom,rpmh-stats driver.
> Yeah I'dve never thought there would be garbage in there..
>
> I'd advocate for simply not doing anything wrt sleep stats if DDR
> stats are unavailable though. The QMP handle can stay, as there
> may (I don't know) be more data available that we want to export
> through this driver.

FWIW, I'm getting a crash on sc7180-trogdor like this too. In kgdb it
says I'm on line:

key = readl(ddrd->base);

...and

(gdb) print ddrd->base
$1 = (void *) 0xffffffc0833a3149
(gdb) print reg
$2 = (void *) 0xffffffc0833a3000

...so I guess my "stats_offset" must have been 0x149.

Can we get a fix landed or a revert? Thanks! :-)

-Doug
Konrad Dybcio Dec. 14, 2023, 11:59 a.m. UTC | #5
On 12/14/23 01:59, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 11, 2023 at 1:11 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 9.12.2023 22:55, Dmitry Baryshkov wrote:
>>> The stats ram on sm8150 platform contains invalid data at the
>>> DDR_DYNAMIC_OFFSET. Most likely this is because the platform didn't
>>> support DDR sleep stats.
>> Interesting. Can you read back DDR_DYNAMIC_OFFSET on 8350/8280 and
>> see if 8150 has correct data in there?
>>
>>> However this platform uses generic
>>> "qcom,rpmh-stats" compatible, which implies presense of the DDR data.
>>> Add safety net to prevent old DTB files from crashing the
>>> qcom,rpmh-stats driver.
>> Yeah I'dve never thought there would be garbage in there..
>>
>> I'd advocate for simply not doing anything wrt sleep stats if DDR
>> stats are unavailable though. The QMP handle can stay, as there
>> may (I don't know) be more data available that we want to export
>> through this driver.
> 
> FWIW, I'm getting a crash on sc7180-trogdor like this too. In kgdb it
> says I'm on line:
> 
> key = readl(ddrd->base);
> 
> ...and
> 
> (gdb) print ddrd->base
> $1 = (void *) 0xffffffc0833a3149
> (gdb) print reg
> $2 = (void *) 0xffffffc0833a3000
> 
> ...so I guess my "stats_offset" must have been 0x149.
> 
> Can we get a fix landed or a revert? Thanks! :-)
Right, I guess we may want to revert it and I'll try to get more
info from Qualcomm folks..

Konrad
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml
index 686a7ef2f48a..9cd75bedc7d2 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml
@@ -21,6 +21,7 @@  properties:
     enum:
       - qcom,rpmh-stats
       - qcom,sdm845-rpmh-stats
+      - qcom,sm8150-rpmh-stats
       - qcom,rpm-stats
       # For older RPM firmware versions with fixed offset for the sleep stats
       - qcom,apq8084-rpm-stats