From: Mahesh Sivasubramanian <[email protected]>
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: [email protected]
Signed-off-by: Mahesh Sivasubramanian <[email protected]>
Signed-off-by: Lina Iyer <[email protected]>
Signed-off-by: Maulik Shah <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
.../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 <[email protected]>
+
+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>;
+ };
+...
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On Thu, Oct 07, 2021 at 03:27:25PM +0530, Maulik Shah wrote:
> From: Mahesh Sivasubramanian <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Mahesh Sivasubramanian <[email protected]>
> Signed-off-by: Lina Iyer <[email protected]>
> Signed-off-by: Maulik Shah <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> Reviewed-by: Stephen Boyd <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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.
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"?
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"?
Thanks,
Stephan
[1]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pm.dtsi?h=LA.UM.8.4.1.c25#n271
Hi Stephan,
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 <[email protected]>
>>
>> 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: [email protected]
>> Signed-off-by: Mahesh Sivasubramanian <[email protected]>
>> Signed-off-by: Lina Iyer <[email protected]>
>> Signed-off-by: Maulik Shah <[email protected]>
>> Reviewed-by: Rob Herring <[email protected]>
>> Reviewed-by: Bjorn Andersson <[email protected]>
>> Reviewed-by: Stephen Boyd <[email protected]>
>> ---
>> .../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 <[email protected]>
>> +
>> +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.
>
> 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.
Thanks,
Maulik
>
> Thanks,
> Stephan
>
> [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pm.dtsi?h=LA.UM.8.4.1.c25#n271
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
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 <[email protected]>
> > >
> > > 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: [email protected]
> > > Signed-off-by: Mahesh Sivasubramanian <[email protected]>
> > > Signed-off-by: Lina Iyer <[email protected]>
> > > Signed-off-by: Maulik Shah <[email protected]>
> > > Reviewed-by: Rob Herring <[email protected]>
> > > Reviewed-by: Bjorn Andersson <[email protected]>
> > > Reviewed-by: Stephen Boyd <[email protected]>
> > > ---
> > > .../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 <[email protected]>
> > > +
> > > +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.
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>;
};
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.
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
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 <[email protected]>
>>>>
>>>> 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: [email protected]
>>>> Signed-off-by: Mahesh Sivasubramanian <[email protected]>
>>>> Signed-off-by: Lina Iyer <[email protected]>
>>>> Signed-off-by: Maulik Shah <[email protected]>
>>>> Reviewed-by: Rob Herring <[email protected]>
>>>> Reviewed-by: Bjorn Andersson <[email protected]>
>>>> Reviewed-by: Stephen Boyd <[email protected]>
>>>> ---
>>>> .../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 <[email protected]>
>>>> +
>>>> +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
Hi Maulik,
On Wed, Oct 13, 2021 at 11:27:30AM +0530, Maulik Shah wrote:
> 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 <[email protected]>
> > > > >
> > > > > 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: [email protected]
> > > > > Signed-off-by: Mahesh Sivasubramanian <[email protected]>
> > > > > Signed-off-by: Lina Iyer <[email protected]>
> > > > > Signed-off-by: Maulik Shah <[email protected]>
> > > > > Reviewed-by: Rob Herring <[email protected]>
> > > > > Reviewed-by: Bjorn Andersson <[email protected]>
> > > > > Reviewed-by: Stephen Boyd <[email protected]>
> > > > > ---
> > > > > .../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 <[email protected]>
> > > > > +
> > > > > +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.
>
Thanks!
> >
> > 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
>
I already tested it on MSM8916 and this is only partially true. :)
It looks like the RPM_DYNAMIC_ADDR was added together with some firmware
update. I tested on several devices and on most of them RPM_DYNAMIC_ADDR
reads 0xdba0 (that is, there are no changes necessary in the driver).
So far I have only found one single device with fairly old firmware
where RPM_DYNAMIC_ADDR reads 0x0.
> 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.
>
But yeah, I guess we might need the fixed offset anyway at some point
for even older targets than MSM8916, so it is probably safer to add it
like this. Will investigate it some more once your patch is accepted.
> 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.
>
Almost, it's "vmin" and "xosd" ("XO Shutdown" I guess) on MSM8916. :)
Thanks,
Stephan