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

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(+)

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

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.

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.

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

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