Messages in this thread | | | Subject | Re: [PATCH v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings | From | Maulik Shah <> | Date | Wed, 13 Oct 2021 11:27:30 +0530 |
| |
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@codeaurora.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@vger.kernel.org >>>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org> >>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >>>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >>>> Reviewed-by: Rob Herring <robh@kernel.org> >>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> >>>> Reviewed-by: Stephen Boyd <swboyd@chromium.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@codeaurora.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@c3f0000 { >>>> + compatible = "qcom,rpmh-sleep-stats"; >>>> + reg = <0x0c3f0000 0x400>; >>>> + }; >>>> + # Example of rpm sleep stats >>>> + - | >>>> + sram@4690000 { >>>> + 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@290000 { > 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@290000 { 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
| |