Message-ID: <c22d57cc-025b-5a8c-91d5-5385292da49f@codeaurora.org>
Date:   Wed, 13 Oct 2021 11:27:30 +0530
From:   Maulik Shah <mkshah@...eaurora.org>
To:     Stephan Gerhold <stephan@...hold.net>
Cc:     swboyd@...omium.org, mka@...omium.org, evgreen@...omium.org,
        bjorn.andersson@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, agross@...nel.org,
        dianders@...omium.org, linux@...ck-us.net, rnayak@...eaurora.org,
        lsrao@...eaurora.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings

Hi Stephan,

On 10/8/2021 4:29 PM, Stephan Gerhold wrote:
> On Fri, Oct 08, 2021 at 02:20:24PM +0530, Maulik Shah wrote:
>> On 10/8/2021 1:20 AM, Stephan Gerhold wrote:
>>> On Thu, Oct 07, 2021 at 03:27:25PM +0530, Maulik Shah wrote:
>>>> From: Mahesh Sivasubramanian <msivasub@...eaurora.org>
>>>>
>>>> Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
>>>> Sleep stats driver. The driver is used for displaying Sleep statistic maintained
>>>> by Always On Processor or Resource Power Manager.
>>>>
>>>> Cc: devicetree@...r.kernel.org
>>>> Signed-off-by: Mahesh Sivasubramanian <msivasub@...eaurora.org>
>>>> Signed-off-by: Lina Iyer <ilina@...eaurora.org>
>>>> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
>>>> Reviewed-by: Rob Herring <robh@...nel.org>
>>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>
>>>> Reviewed-by: Stephen Boyd <swboyd@...omium.org>
>>>> ---
>>>>    .../bindings/soc/qcom/qcom-sleep-stats.yaml        | 47 ++++++++++++++++++++++
>>>>    1 file changed, 47 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>>>> new file mode 100644
>>>> index 0000000..5213daf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>>>> @@ -0,0 +1,47 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/soc/qcom/qcom-sleep-stats.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Qualcomm Technologies, Inc. (QTI) Sleep stats bindings
>>>> +
>>>> +maintainers:
>>>> +  - Maulik Shah <mkshah@...eaurora.org>
>>>> +
>>>> +description:
>>>> +  Always On Processor/Resource Power Manager maintains statistics of the SoC
>>>> +  sleep modes involving powering down of the rails and oscillator clock.
>>>> +
>>>> +  Statistics includes SoC sleep mode type, number of times low power mode were
>>>> +  entered, time of last entry, time of last exit and accumulated sleep duration.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - qcom,rpmh-sleep-stats
>>>> +      - qcom,rpm-sleep-stats
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  # Example of rpmh sleep stats
>>>> +  - |
>>>> +    sram@...0000 {
>>>> +      compatible = "qcom,rpmh-sleep-stats";
>>>> +      reg = <0x0c3f0000 0x400>;
>>>> +    };
>>>> +  # Example of rpm sleep stats
>>>> +  - |
>>>> +    sram@...0000 {
>>>> +      compatible = "qcom,rpm-sleep-stats";
>>>> +      reg = <0x04690000 0x400>;
>>>> +    };
>>>
>>> Does this region really only contain "rpm-*sleep*-stats"? AFAICT this is
>>> really a more generic memory region where various offsets are read from.
>>
>> Right the area contains all kind of sleep stats/logs.
>>
>>>
>>> These are all the offsets in msm8998-pm.dtsi downstream [1]:
>>>     ...9000c: rpm-rail-stats offset
>>>     ...90014: rpm-sleep-stats offset (RPM_DYNAMIC_ADDR in your driver)
>>>     ...90018: rpm-log offset
>>>     ...9001c: "RPM FREE HEAP SPACE"
>>>
>>> How would you set up any of the other drivers if the entire region
>>> is declared as "rpm-sleep-stats"?
>>
>> We don't need to setup other drivers.
>> The idea is to have single stats driver (qcom_sleep_stats) that can be
>> enahanced to to read other stats also on both RPM/ PRMH targets.
>>
>> Today this driver reads only sleep stats from offset 0x90014.
>> if in future say, we want to read rpm-rail-stats from offset 0x9000c then it
>> can be added in the existing driver.
>>
>> In the rpm data, add one more entry to indicate reading rpm-rail-stats,
>> something like below and add new compatible flag for the target, may be
>> "qcom,rpm-sleep-stats-legacy" and should work like,
>>
>> #define RPM_RAIL_STATS_ADDR 0xc
>>
>> static const struct stats_config rpm_data = {
>>          .stats_offset = 0,
>>          .num_records = 2,
>>          .appended_stats_avail = true,
>>          .dynamic_offset = true,
>>          .rpm_rail_stats = true,
>> };
>>
>> and check in driver probe like
>>
>>          if (config->rpm_rail_stats) {
>> 	//create rpm rail stats related files.
>>          }
>>
>> Similarly for other rpm-logs / any other stats can be added in same driver.
>>
>> Hope this clarifies.
>>
> 
> Having the same driver for all this sounds good, thanks for clarifying!
> 
>>>
>>> Perhaps this region should have a more generic name that represents what
>>> it actually is and not only one of the information it contains, similar
>>> to "qcom,rpm-msg-ram"?
>>
>> sram looks good to me. Actually on RPM targets its RPM data ram and RPMH
>> targets its msgram. To keep the name common sram is used here.
>>
> 
> The node name (sram@...) is fine. I was actually referring to the
> "compatible". Given what you have written above there is more in that
> region than just "qcom,rpm-*sleep*-stats". There are also logs and other
> stats there. Perhaps "qcom,rpm(h)-stats" (without -sleep-) would already
> be more clear.

ok updated in v12 to name it qcom,rpm(h)-stats. similarly driver name 
also updated from qcom_sleep_stats to qcom_stats.

> 
> Or, my other suggestion was to not make any assumption about particular
> contents of the data RAM in the device tree. The compatible could also
> be called simply "qcom,rpm-data-ram" (just like "qcom,rpm-msg-ram") and
> the assumption what it contains (e.g. sleep stats) could be made in your
> existing driver.
> 
> This is probably a matter of preference at the end and won't make a big
> difference but I just wanted to mention it.
> 
> For example, to make your driver work on MSM8916 I had to declare a much
> larger memory region (e.g. 0x10000 vs 0x400):
> 
> 		sram@...000 {
> 			compatible = "qcom,rpm-sleep-stats";
> 			reg = <0x00290000 0x10000>;
> 		};
> 

the driver can not be as is enabled for MSM8916. Actually on MSM8916 
(even though its RPM target) it uses fixed offset 0xdba0 from the base 
address in the devicetree.

> This is because RPM_DYNAMIC_ADDR contains the offset 0xdba0 instead of
> e.g. 0x020c on newer SoCs and the memory region must be large enough to
> cover the offset.

On all other RPM targets followed by MSM8916 the address for soc sleep 
stats is present at offset 0x14 (RPM_DYNAMIC_ADDR ) from the base 
address but not on MSM8916. so MSM8916 is a special case to enable for 
for RPM stats

To enable this for MSM8916, dynamic_offset need to be false and 
stats_offset need to be given directly as 0xdba0, something similar as 
RPMH targets.

static const struct stats_config rpm_legacy_data = {
         .stats_offset = 0xdba0,
         .num_records = 2,
         .appended_stats_avail = true,
         .dynamic_offset = false,
         .subsystem_stats_in_smem = false,
};

{ .compatible = "qcom,rpm-legacy-stats", .data = &rpm_legacy_data },

and then add

                 sram@...000 {
                         compatible = "qcom,rpm-legacy-stats";
                         reg = <0x00290000 0x10000>;
                 };

which should work fine.

I donot have MSM8916 to validate this but when this driver boots up it 
should create "vlow" and "vmin" files at /sys/kernel/debug/qcom_sleep_stats/
if that is done, it means you are reading stats from correct address 
since the file name is read from the memory itself.

Thanks,
Maulik

> 
> It just feels a bit weird for me to describe such a large region as only
> containing "qcom,rpm-*sleep*-stats". "qcom,rpm-data-ram" might better
> describe it what it really is and then your driver can decide what to do
> with it.
> 
> But, as I said, this is just naming which is always subjective. :)
> 
> Thanks!
> Stephan
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of Code Aurora Forum, hosted by The Linux Foundation

Powered by blists - more mailing lists