* [PATCH v2 0/4] Introduce SoC sleep stats driver @ 2020-02-21 8:49 Maulik Shah 2020-02-21 8:49 ` [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Maulik Shah @ 2020-02-21 8:49 UTC (permalink / raw) To: swboyd, mka, evgreen, bjorn.andersson Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Maulik Shah Changes in v2: - Convert Documentation to YAML. - Address stephen's comments from v1. - Use debugfs instead of sysfs. - Add sc7180 dts changes for sleep stats - Add defconfig changes to enable driver - Include subsystem stats from [1] in this single stats driver. - Address stephen's comments from [1] - Update cover letter inline to mention [1] Qualcomm Technologies, Inc. (QTI)'s chipsets support SoC level low power modes. SoCs Always On Processor/Resource Power Manager produces statistics of the SoC sleep modes involving lowering or powering down of the rails and the oscillator clock. Additionally multiple subsystems present on SoC like modem, spss, adsp, cdsp maintains their low power mode statistics in shared memory (SMEM). Statistics includes SoC sleep mode type, number of times LPM entered, time of last entry, exit, and accumulated sleep duration in seconds. This series adds a driver to read the stats and export to debugfs. [1] https://lore.kernel.org/patchwork/patch/1149381/ Mahesh Sivasubramanian (2): dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs soc: qcom: Add SoC sleep stats driver Maulik Shah (2): arm64: dts: qcom: sc7180: Enable soc sleep stats arm64: defconfig: Enable SoC sleep stats driver for Qualcomm .../bindings/soc/qcom/soc-sleep-stats.yaml | 47 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 + arch/arm64/configs/defconfig | 1 + drivers/soc/qcom/Kconfig | 10 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/soc_sleep_stats.c | 279 +++++++++++++++++++++ 6 files changed, 343 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml create mode 100644 drivers/soc/qcom/soc_sleep_stats.c -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2020-02-21 8:49 [PATCH v2 0/4] Introduce SoC sleep stats driver Maulik Shah @ 2020-02-21 8:49 ` Maulik Shah 2020-02-26 20:05 ` Rob Herring ` (2 more replies) 2020-02-21 8:49 ` [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver Maulik Shah ` (2 subsequent siblings) 3 siblings, 3 replies; 15+ messages in thread From: Maulik Shah @ 2020-02-21 8:49 UTC (permalink / raw) To: swboyd, mka, evgreen, bjorn.andersson Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, devicetree, Maulik Shah From: Mahesh Sivasubramanian <msivasub@codeaurora.org> Add device binding documentation for Qualcomm Technology Inc's (QTI) SoC sleep stats driver. The driver is used for displaying SoC 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> --- .../bindings/soc/qcom/soc-sleep-stats.yaml | 47 ++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml new file mode 100644 index 00000000..50352a4 --- /dev/null +++ b/Documentation/devicetree/bindings/soc/qcom/soc-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/bindings/soc/qcom/soc-sleep-stats.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies, Inc. (QTI) SoC sleep stats bindings + +maintainers: + - Maulik Shah <mkshah@codeaurora.org> + - Lina Iyer <ilina@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. + SoC sleep stats driver provides debugfs interface to show this information. + +properties: + compatible: + enum: + - qcom,rpmh-sleep-stats + - qcom,rpm-sleep-stats + + reg: + maxItems: 1 + +required: + - compatible + - reg + +examples: + # Example of rpmh sleep stats + - | + rpmh_sleep_stats: soc-sleep-stats@c3f0000 { + compatible = "qcom,rpmh-sleep-stats"; + reg = <0 0xc3f0000 0 0x400>; + }; + # Example of rpm sleep stats + - | + rpm_sleep_stats: soc-sleep-stats@4690000 { + compatible = "qcom,rpm-sleep-stats"; + reg = <0 0x04690000 0 0x400>; + }; +... -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2020-02-21 8:49 ` [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah @ 2020-02-26 20:05 ` Rob Herring 2020-02-28 6:38 ` Bjorn Andersson 2020-02-28 16:47 ` Stephen Boyd 2 siblings, 0 replies; 15+ messages in thread From: Rob Herring @ 2020-02-26 20:05 UTC (permalink / raw) To: Maulik Shah Cc: swboyd, mka, evgreen, bjorn.andersson, linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, devicetree, Maulik Shah On Fri, 21 Feb 2020 14:19:43 +0530, Maulik Shah wrote: > From: Mahesh Sivasubramanian <msivasub@codeaurora.org> > > Add device binding documentation for Qualcomm Technology Inc's (QTI) > SoC sleep stats driver. The driver is used for displaying SoC 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> > --- > .../bindings/soc/qcom/soc-sleep-stats.yaml | 47 ++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2020-02-21 8:49 ` [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah 2020-02-26 20:05 ` Rob Herring @ 2020-02-28 6:38 ` Bjorn Andersson 2020-02-28 16:47 ` Stephen Boyd 2 siblings, 0 replies; 15+ messages in thread From: Bjorn Andersson @ 2020-02-28 6:38 UTC (permalink / raw) To: Maulik Shah Cc: swboyd, mka, evgreen, linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, devicetree On Fri 21 Feb 00:49 PST 2020, Maulik Shah wrote: > From: Mahesh Sivasubramanian <msivasub@codeaurora.org> > > Add device binding documentation for Qualcomm Technology Inc's (QTI) > SoC sleep stats driver. The driver is used for displaying SoC sleep > statistic maintained by Always On Processor or Resource Power Manager. > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > 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> > --- > .../bindings/soc/qcom/soc-sleep-stats.yaml | 47 ++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml > > diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml > new file mode 100644 > index 00000000..50352a4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/qcom/soc-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/bindings/soc/qcom/soc-sleep-stats.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Technologies, Inc. (QTI) SoC sleep stats bindings > + > +maintainers: > + - Maulik Shah <mkshah@codeaurora.org> > + - Lina Iyer <ilina@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. > + SoC sleep stats driver provides debugfs interface to show this information. > + > +properties: > + compatible: > + enum: > + - qcom,rpmh-sleep-stats > + - qcom,rpm-sleep-stats > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +examples: > + # Example of rpmh sleep stats > + - | > + rpmh_sleep_stats: soc-sleep-stats@c3f0000 { > + compatible = "qcom,rpmh-sleep-stats"; > + reg = <0 0xc3f0000 0 0x400>; > + }; > + # Example of rpm sleep stats > + - | > + rpm_sleep_stats: soc-sleep-stats@4690000 { > + compatible = "qcom,rpm-sleep-stats"; > + reg = <0 0x04690000 0 0x400>; > + }; > +... > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2020-02-21 8:49 ` [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah 2020-02-26 20:05 ` Rob Herring 2020-02-28 6:38 ` Bjorn Andersson @ 2020-02-28 16:47 ` Stephen Boyd 2020-03-06 6:22 ` Maulik Shah 2 siblings, 1 reply; 15+ messages in thread From: Stephen Boyd @ 2020-02-28 16:47 UTC (permalink / raw) To: Maulik Shah, bjorn.andersson, evgreen, mka Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, devicetree, Maulik Shah Quoting Maulik Shah (2020-02-21 00:49:43) > diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml > new file mode 100644 > index 00000000..50352a4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/qcom/soc-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/bindings/soc/qcom/soc-sleep-stats.yaml# Drop 'bindings' from above? > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Technologies, Inc. (QTI) SoC sleep stats bindings > + > +maintainers: > + - Maulik Shah <mkshah@codeaurora.org> > + - Lina Iyer <ilina@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. > + SoC sleep stats driver provides debugfs interface to show this information. Please remove this last line. It is a Linuxism that doesn't belong in DT bindings. And then make it one paragraph and drop the | because formatting doesn't need to be maintained. > + > +properties: > + compatible: > + enum: > + - qcom,rpmh-sleep-stats > + - qcom,rpm-sleep-stats > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +examples: > + # Example of rpmh sleep stats > + - | > + rpmh_sleep_stats: soc-sleep-stats@c3f0000 { > + compatible = "qcom,rpmh-sleep-stats"; > + reg = <0 0xc3f0000 0 0x400>; > + }; I see that on sc7180 aoss-qmp is overlapping with this region. aoss_qmp: qmp@c300000 { compatible = "qcom,sc7180-aoss-qmp"; reg = <0 0x0c300000 0 0x100000>; So is this register region really something more like a TCM or RAM area where rpmh combines multiple software concepts into one hardware memory region? The aoss-qmp driver talks about message RAM, so I think this sleep stats stuff is a carveout of the RPMh message RAM. It seems OK if we want to split that message RAM up into multiple DT nodes, but then we'll need to reduce the reg size for the aoss-qmp node. Finally, the node name 'soc-sleep-stats' is generic, but I wonder why we couldn't name the node 'tcm' or 'memory' or 'msgram'. Similarly for the qmp node it fits better DT style to have the node be something generic. > + # Example of rpm sleep stats > + - | > + rpm_sleep_stats: soc-sleep-stats@4690000 { > + compatible = "qcom,rpm-sleep-stats"; > + reg = <0 0x04690000 0 0x400>; > + }; > +... ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2020-02-28 16:47 ` Stephen Boyd @ 2020-03-06 6:22 ` Maulik Shah 0 siblings, 0 replies; 15+ messages in thread From: Maulik Shah @ 2020-03-06 6:22 UTC (permalink / raw) To: Stephen Boyd, bjorn.andersson, evgreen, mka Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, devicetree On 2/28/2020 10:17 PM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-02-21 00:49:43) >> diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml >> new file mode 100644 >> index 00000000..50352a4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-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/bindings/soc/qcom/soc-sleep-stats.yaml# > Drop 'bindings' from above? Done. >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm Technologies, Inc. (QTI) SoC sleep stats bindings >> + >> +maintainers: >> + - Maulik Shah <mkshah@codeaurora.org> >> + - Lina Iyer <ilina@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. >> + SoC sleep stats driver provides debugfs interface to show this information. > Please remove this last line. It is a Linuxism that doesn't belong in DT > bindings. And then make it one paragraph and drop the | because > formatting doesn't need to be maintained. Done. >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,rpmh-sleep-stats >> + - qcom,rpm-sleep-stats >> + >> + reg: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + >> +examples: >> + # Example of rpmh sleep stats >> + - | >> + rpmh_sleep_stats: soc-sleep-stats@c3f0000 { >> + compatible = "qcom,rpmh-sleep-stats"; >> + reg = <0 0xc3f0000 0 0x400>; >> + }; > I see that on sc7180 aoss-qmp is overlapping with this region. > > > aoss_qmp: qmp@c300000 { > compatible = "qcom,sc7180-aoss-qmp"; > reg = <0 0x0c300000 0 0x100000>; > > So is this register region really something more like a TCM or RAM area > where rpmh combines multiple software concepts into one hardware memory > region? The aoss-qmp driver talks about message RAM, so I think this > sleep stats stuff is a carveout of the RPMh message RAM. It seems OK if > we want to split that message RAM up into multiple DT nodes, but then > we'll need to reduce the reg size for the aoss-qmp node. Yes, i discussed this long back with bjorn, we will limit aoss_qmp size to 0x400. Will include in next revision in DTSI change. > Finally, the node name 'soc-sleep-stats' is generic, but I wonder why we > couldn't name the node 'tcm' or 'memory' or 'msgram'. Similarly for the > qmp node it fits better DT style to have the node be something generic. following DTSI change in this series has comment to drop the label, since no one references this node. if we drop label than just naming it msgram@c3f0000 will also be too generic IMO. To address both the comments i am planning to drop label and also soc-sleep-stats and just keep rpmh_sleep_stats@c3f0000. Thanks, Maulik >> + # Example of rpm sleep stats >> + - | >> + rpm_sleep_stats: soc-sleep-stats@4690000 { >> + compatible = "qcom,rpm-sleep-stats"; >> + reg = <0 0x04690000 0 0x400>; >> + }; >> +... -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver 2020-02-21 8:49 [PATCH v2 0/4] Introduce SoC sleep stats driver Maulik Shah 2020-02-21 8:49 ` [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah @ 2020-02-21 8:49 ` Maulik Shah 2020-02-28 21:10 ` Stephen Boyd 2020-02-21 8:49 ` [PATCH v2 3/4] arm64: dts: qcom: sc7180: Enable soc sleep stats Maulik Shah 2020-02-21 8:49 ` [PATCH v2 4/4] arm64: defconfig: Enable SoC sleep stats driver for Qualcomm Maulik Shah 3 siblings, 1 reply; 15+ messages in thread From: Maulik Shah @ 2020-02-21 8:49 UTC (permalink / raw) To: swboyd, mka, evgreen, bjorn.andersson Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, Maulik Shah From: Mahesh Sivasubramanian <msivasub@codeaurora.org> Lets's add a driver to read the the stats from remote processor and export to debugfs. The driver creates "soc_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with command cat /sys/kernel/debug/soc_sleep_stats/ddr count = 0 Last Entered At = 0 Last Exited At = 0 Accumulated Duration = 0 Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org> Signed-off-by: Lina Iyer <ilina@codeaurora.org> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- drivers/soc/qcom/Kconfig | 10 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/soc_sleep_stats.c | 279 +++++++++++++++++++++++++++++++++++++ 3 files changed, 290 insertions(+) create mode 100644 drivers/soc/qcom/soc_sleep_stats.c diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index d0a73e7..4d36654 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -185,6 +185,16 @@ config QCOM_SOCINFO Say yes here to support the Qualcomm socinfo driver, providing information about the SoC to user space. +config QCOM_SOC_SLEEP_STATS + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver" + depends on ARCH_QCOM + depends on DEBUG_FS + help + Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver to read + the shared memory exported by the remote processor related to + various SoC level low power modes statistics and export to debugfs + interface. + config QCOM_WCNSS_CTRL tristate "Qualcomm WCNSS control driver" depends on ARCH_QCOM || COMPILE_TEST diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 9fb35c8..e6bd052 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o +obj-$(CONFIG_QCOM_SOC_SLEEP_STATS) += soc_sleep_stats.o obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o obj-$(CONFIG_QCOM_APR) += apr.o obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c new file mode 100644 index 00000000..bf38bb5 --- /dev/null +++ b/drivers/soc/qcom/soc_sleep_stats.c @@ -0,0 +1,279 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2011-2020, The Linux Foundation. All rights reserved. + */ + +#include <linux/device.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/uaccess.h> +#include <linux/debugfs.h> +#include <linux/seq_file.h> +#include <linux/stddef.h> + +#include <linux/soc/qcom/smem.h> +#include <clocksource/arm_arch_timer.h> + +#define NAME_LENGTH 5 + +enum subsystem_item_id { + MODEM = 605, + ADSP, + CDSP, + SLPI, + GPU, + DISPLAY, +}; + +enum subsystem_pid { + PID_APSS = 0, + PID_MODEM = 1, + PID_ADSP = 2, + PID_SLPI = 3, + PID_CDSP = 5, + PID_GPU = PID_APSS, + PID_DISPLAY = PID_APSS, +}; + +struct subsystem_data { + char *name; + enum subsystem_item_id item_id; + enum subsystem_pid pid; +}; + +static struct subsystem_data subsystems[] = { + { "modem", MODEM, PID_MODEM }, + { "adsp", ADSP, PID_ADSP }, + { "cdsp", CDSP, PID_CDSP }, + { "slpi", SLPI, PID_SLPI }, + { "gpu", GPU, PID_GPU }, + { "display", DISPLAY, PID_DISPLAY }, +}; + +struct stats_config { + u32 offset_addr; + u32 num_records; + bool appended_stats_avail; +}; + +static const struct stats_config *config; + +struct soc_sleep_stats_data { + phys_addr_t stats_base; + resource_size_t stats_size; + void __iomem *reg; + struct dentry *root; +}; + +struct entry { + __le32 stat_type; + __le32 count; + __le64 last_entered_at; + __le64 last_exited_at; + __le64 accumulated; +}; + +struct appended_entry { + __le32 client_votes; + __le32 reserved[3]; +}; + +static u64 get_time_in_sec(u64 counter) +{ + do_div(counter, arch_timer_get_rate()); + + return counter; +} + +static void print_sleep_stats(struct seq_file *s, struct entry *e) +{ + e->last_entered_at = get_time_in_sec(e->last_entered_at); + e->last_exited_at = get_time_in_sec(e->last_exited_at); + e->accumulated = get_time_in_sec(e->accumulated); + + seq_printf(s, "count = %u\n", e->count); + seq_printf(s, "Last Entered At = %llu\n", e->last_entered_at); + seq_printf(s, "Last Exited At = %llu\n", e->last_exited_at); + seq_printf(s, "Accumulated Duration = %llu\n", e->accumulated); +} + +static int subsystem_sleep_stats_show(struct seq_file *s, void *d) +{ + struct subsystem_data *subsystem = s->private; + struct entry *e; + + e = qcom_smem_get(subsystem->pid, subsystem->item_id, NULL); + if (IS_ERR(e)) + return PTR_ERR(e); + + /* + * If a subsystem is in sleep when reading the sleep stats adjust + * the accumulated sleep duration to show actual sleep time. + */ + if (e->last_entered_at > e->last_exited_at) + e->accumulated += (arch_timer_read_counter() + - e->last_entered_at); + + print_sleep_stats(s, e); + + return 0; +} + +static int soc_sleep_stats_show(struct seq_file *s, void *d) +{ + void __iomem *reg = s->private; + uint32_t offset; + struct entry e; + + offset = offsetof(struct entry, count); + e.count = le32_to_cpu(readl_relaxed(reg + offset)); + + offset = offsetof(struct entry, last_entered_at); + e.last_entered_at = le64_to_cpu(readq_relaxed(reg + offset)); + + offset = offsetof(struct entry, last_exited_at); + e.last_exited_at = le64_to_cpu(readq_relaxed(reg + offset)); + + offset = offsetof(struct entry, accumulated); + e.accumulated = le64_to_cpu(readq_relaxed(reg + offset)); + + print_sleep_stats(s, &e); + + if (config->appended_stats_avail) { + struct appended_entry ae; + + offset = offsetof(struct appended_entry, client_votes) + + sizeof(struct entry); + ae.client_votes = le32_to_cpu(readl_relaxed(reg + offset)); + seq_printf(s, "Client_votes = 0x%x\n", ae.client_votes); + } + + return 0; +} + +DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats); +DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats); + +static const struct stats_config rpm_data = { + .offset_addr = 0x14, + .num_records = 2, + .appended_stats_avail = true, +}; + +static const struct stats_config rpmh_data = { + .offset_addr = 0x4, + .num_records = 3, + .appended_stats_avail = false, +}; + +static const struct of_device_id soc_sleep_stats_table[] = { + { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data}, + { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data}, + { }, +}; + +static int create_debugfs_entries(struct soc_sleep_stats_data *drv) +{ + struct entry *e; + char stat_type[NAME_LENGTH] = {0}; + uint32_t offset, type; + int i; + + drv->root = debugfs_create_dir("soc_sleep_stats", NULL); + if (IS_ERR_OR_NULL(drv->root)) + return PTR_ERR(drv->root); + + for (i = 0; i < config->num_records; i++) { + offset = offsetof(struct entry, stat_type) + + (i * sizeof(struct entry)); + + if (config->appended_stats_avail) + offset += i * sizeof(struct appended_entry); + + type = le32_to_cpu(readl_relaxed(drv->reg + offset)); + memcpy(stat_type, &type, sizeof(uint32_t)); + debugfs_create_file(stat_type, 0444, drv->root, + drv->reg + offset, + &soc_sleep_stats_fops); + } + + for (i = 0; i < ARRAY_SIZE(subsystems); i++) { + e = qcom_smem_get(subsystems[i].pid, subsystems[i].item_id, + NULL); + + if (IS_ERR(e)) + continue; + + debugfs_create_file(subsystems[i].name, 0444, drv->root, + &subsystems[i], + &subsystem_sleep_stats_fops); + } + + return 0; +} + +static int soc_sleep_stats_probe(struct platform_device *pdev) +{ + struct soc_sleep_stats_data *drv; + struct resource *res; + void __iomem *offset_addr; + + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); + if (!drv) + return -ENOMEM; + + config = of_device_get_match_data(&pdev->dev); + if (!config) + return -ENODEV; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return PTR_ERR(res); + + offset_addr = ioremap_nocache(res->start + config->offset_addr, + sizeof(u32)); + if (IS_ERR(offset_addr)) + return PTR_ERR(offset_addr); + + drv->stats_base = res->start | readl_relaxed(offset_addr); + drv->stats_size = resource_size(res); + iounmap(offset_addr); + + drv->reg = devm_ioremap(&pdev->dev, drv->stats_base, drv->stats_size); + if (!drv->reg) + return -ENOMEM; + + if (create_debugfs_entries(drv)) + return -ENODEV; + + platform_set_drvdata(pdev, drv); + + return 0; +} + +static int soc_sleep_stats_remove(struct platform_device *pdev) +{ + struct soc_sleep_stats_data *drv = platform_get_drvdata(pdev); + + debugfs_remove_recursive(drv->root); + + return 0; +} + +static struct platform_driver soc_sleep_stats_driver = { + .probe = soc_sleep_stats_probe, + .remove = soc_sleep_stats_remove, + .driver = { + .name = "soc_sleep_stats", + .of_match_table = soc_sleep_stats_table, + }, +}; +module_platform_driver(soc_sleep_stats_driver); + +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC Sleep Stats driver"); +MODULE_LICENSE("GPL v2"); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver 2020-02-21 8:49 ` [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver Maulik Shah @ 2020-02-28 21:10 ` Stephen Boyd 2020-03-06 6:35 ` Maulik Shah 0 siblings, 1 reply; 15+ messages in thread From: Stephen Boyd @ 2020-02-28 21:10 UTC (permalink / raw) To: Maulik Shah, bjorn.andersson, evgreen, mka Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, Maulik Shah Quoting Maulik Shah (2020-02-21 00:49:44) > From: Mahesh Sivasubramanian <msivasub@codeaurora.org> > > Lets's add a driver to read the the stats from remote processor Let's > and export to debugfs. > > The driver creates "soc_sleep_stats" directory in debugfs and adds > files for various low power mode available. Below is sample output > with command > > cat /sys/kernel/debug/soc_sleep_stats/ddr > count = 0 > Last Entered At = 0 > Last Exited At = 0 > Accumulated Duration = 0 > > Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org> > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > drivers/soc/qcom/Kconfig | 10 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/soc_sleep_stats.c | 279 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 290 insertions(+) > create mode 100644 drivers/soc/qcom/soc_sleep_stats.c > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index d0a73e7..4d36654 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -185,6 +185,16 @@ config QCOM_SOCINFO > Say yes here to support the Qualcomm socinfo driver, providing > information about the SoC to user space. > > +config QCOM_SOC_SLEEP_STATS > + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver" > + depends on ARCH_QCOM Can you add || COMPILE_TEST? > + depends on DEBUG_FS > + help > + Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver to read > + the shared memory exported by the remote processor related to > + various SoC level low power modes statistics and export to debugfs > + interface. > + > config QCOM_WCNSS_CTRL > tristate "Qualcomm WCNSS control driver" > depends on ARCH_QCOM || COMPILE_TEST > diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c > new file mode 100644 > index 00000000..bf38bb5 > --- /dev/null > +++ b/drivers/soc/qcom/soc_sleep_stats.c > @@ -0,0 +1,279 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2011-2020, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/uaccess.h> Why? > +#include <linux/debugfs.h> > +#include <linux/seq_file.h> > +#include <linux/stddef.h> Why? > + > +#include <linux/soc/qcom/smem.h> > +#include <clocksource/arm_arch_timer.h> > + > +#define NAME_LENGTH 5 > + > +enum subsystem_item_id { > + MODEM = 605, > + ADSP, > + CDSP, > + SLPI, > + GPU, > + DISPLAY, > +}; > + > +enum subsystem_pid { > + PID_APSS = 0, > + PID_MODEM = 1, > + PID_ADSP = 2, > + PID_SLPI = 3, > + PID_CDSP = 5, > + PID_GPU = PID_APSS, > + PID_DISPLAY = PID_APSS, I still don't understand this enums. Why not make them a #define set with known values? > +}; > + > +struct subsystem_data { > + char *name; Can it be const char *? > + enum subsystem_item_id item_id; > + enum subsystem_pid pid; > +}; > + > +static struct subsystem_data subsystems[] = { > + { "modem", MODEM, PID_MODEM }, > + { "adsp", ADSP, PID_ADSP }, > + { "cdsp", CDSP, PID_CDSP }, > + { "slpi", SLPI, PID_SLPI }, > + { "gpu", GPU, PID_GPU }, > + { "display", DISPLAY, PID_DISPLAY }, > +}; > + > +struct stats_config { > + u32 offset_addr; Looks ok but probably can just be unsigned int? > + u32 num_records; Why not size_t? Is 32-bit width important? > + bool appended_stats_avail; > +}; > + > +static const struct stats_config *config; This shouldn't be necessary. Can the config be passed into the debugfs file ops private data? > + > +struct soc_sleep_stats_data { This struct is only used in probe. Why not just make probe have more local variables? > + phys_addr_t stats_base; > + resource_size_t stats_size; > + void __iomem *reg; > + struct dentry *root; > +}; > + > +struct entry { > + __le32 stat_type; > + __le32 count; > + __le64 last_entered_at; > + __le64 last_exited_at; > + __le64 accumulated; These should just be u32 and u64. > +}; > + > +struct appended_entry { > + __le32 client_votes; > + __le32 reserved[3]; > +}; Same, just u32. > + > +static u64 get_time_in_sec(u64 counter) > +{ > + do_div(counter, arch_timer_get_rate()); I don't think arch_timer_get_rate() is exported as a symbol. Why we need to convert this to time? Is the counter not sufficient to understand that so many ticks have passed since it entered and exited? > + > + return counter; > +} > + > +static void print_sleep_stats(struct seq_file *s, struct entry *e) > +{ > + e->last_entered_at = get_time_in_sec(e->last_entered_at); > + e->last_exited_at = get_time_in_sec(e->last_exited_at); > + e->accumulated = get_time_in_sec(e->accumulated); > + > + seq_printf(s, "count = %u\n", e->count); > + seq_printf(s, "Last Entered At = %llu\n", e->last_entered_at); > + seq_printf(s, "Last Exited At = %llu\n", e->last_exited_at); > + seq_printf(s, "Accumulated Duration = %llu\n", e->accumulated); Why is count not capitalized but everything else is? > +} > + > +static int subsystem_sleep_stats_show(struct seq_file *s, void *d) > +{ > + struct subsystem_data *subsystem = s->private; > + struct entry *e; > + > + e = qcom_smem_get(subsystem->pid, subsystem->item_id, NULL); Do we need to look this up each time we read the stats? Why can't we get this once in probe or when we create the debugfs file? > + if (IS_ERR(e)) > + return PTR_ERR(e); > + > + /* > + * If a subsystem is in sleep when reading the sleep stats adjust > + * the accumulated sleep duration to show actual sleep time. > + */ > + if (e->last_entered_at > e->last_exited_at) > + e->accumulated += (arch_timer_read_counter() > + - e->last_entered_at); > + > + print_sleep_stats(s, e); > + > + return 0; > +} > + > +static int soc_sleep_stats_show(struct seq_file *s, void *d) > +{ > + void __iomem *reg = s->private; > + uint32_t offset; > + struct entry e; > + > + offset = offsetof(struct entry, count); > + e.count = le32_to_cpu(readl_relaxed(reg + offset)); readl APIs already do endian conversions. > + > + offset = offsetof(struct entry, last_entered_at); > + e.last_entered_at = le64_to_cpu(readq_relaxed(reg + offset)); > + > + offset = offsetof(struct entry, last_exited_at); > + e.last_exited_at = le64_to_cpu(readq_relaxed(reg + offset)); > + > + offset = offsetof(struct entry, accumulated); > + e.accumulated = le64_to_cpu(readq_relaxed(reg + offset)); > + > + print_sleep_stats(s, &e); > + > + if (config->appended_stats_avail) { > + struct appended_entry ae; > + > + offset = offsetof(struct appended_entry, client_votes) + > + sizeof(struct entry); > + ae.client_votes = le32_to_cpu(readl_relaxed(reg + offset)); > + seq_printf(s, "Client_votes = 0x%x\n", ae.client_votes); Use %#x to avoid having to add 0x prefix. Also, can we replace '=' with ':' so that readability is a bit nicer? > + } > + > + return 0; > +} > + > +DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats); > +DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats); > + > +static const struct stats_config rpm_data = { > + .offset_addr = 0x14, > + .num_records = 2, > + .appended_stats_avail = true, > +}; > + > +static const struct stats_config rpmh_data = { > + .offset_addr = 0x4, > + .num_records = 3, > + .appended_stats_avail = false, > +}; > + > +static const struct of_device_id soc_sleep_stats_table[] = { > + { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data}, > + { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data}, > + { }, Please drop comma after sentinel. It makes a compiler error appear if anything comes after. > +}; Move this table next to the driver structure please. > + > +static int create_debugfs_entries(struct soc_sleep_stats_data *drv) > +{ > + struct entry *e; > + char stat_type[NAME_LENGTH] = {0}; Is it a string? Otherwise, seems pretty useless to initialize this to 0 on the stack. > + uint32_t offset, type; Just use u32 instead of uint32_t in any kernel code. > + int i; > + > + drv->root = debugfs_create_dir("soc_sleep_stats", NULL); > + if (IS_ERR_OR_NULL(drv->root)) > + return PTR_ERR(drv->root); This is wrong. Debugfs checks have generally been removed because it's not a problem if debugfs fails. When it fails, drv->root will be NULL and any debugfs_create() function will ignore it. Since this driver depends on DEBUGFS being enabled, -ENODEV won't be returned as an error pointer. > + > + for (i = 0; i < config->num_records; i++) { > + offset = offsetof(struct entry, stat_type) + > + (i * sizeof(struct entry)); This offset of stuff is sort of complicated. I'd prefer to treat it like how we treat most registers with #defines and base + #define sort of logic. That is easier to read. It also let's us decide to completely reorder struct members and not have to worry that the struct doesn't match how memory is laid out. Instead we have macros that define the offset from some base __iomem pointer. > + > + if (config->appended_stats_avail) > + offset += i * sizeof(struct appended_entry); > + > + type = le32_to_cpu(readl_relaxed(drv->reg + offset)); > + memcpy(stat_type, &type, sizeof(uint32_t)); > + debugfs_create_file(stat_type, 0444, drv->root, > + drv->reg + offset, > + &soc_sleep_stats_fops); > + } > + > + for (i = 0; i < ARRAY_SIZE(subsystems); i++) { > + e = qcom_smem_get(subsystems[i].pid, subsystems[i].item_id, > + NULL); > + > + if (IS_ERR(e)) > + continue; > + > + debugfs_create_file(subsystems[i].name, 0444, drv->root, > + &subsystems[i], > + &subsystem_sleep_stats_fops); > + } > + > + return 0; > +} > + > +static int soc_sleep_stats_probe(struct platform_device *pdev) > +{ > + struct soc_sleep_stats_data *drv; > + struct resource *res; > + void __iomem *offset_addr; > + > + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); > + if (!drv) > + return -ENOMEM; > + > + config = of_device_get_match_data(&pdev->dev); Use device_get_match_data() to make this be less DT specific. > + if (!config) > + return -ENODEV; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return PTR_ERR(res); > + > + offset_addr = ioremap_nocache(res->start + config->offset_addr, Why do we need ioremap_nocache()? Can ioremap() work? > + sizeof(u32)); > + if (IS_ERR(offset_addr)) > + return PTR_ERR(offset_addr); > + > + drv->stats_base = res->start | readl_relaxed(offset_addr); Why | vs. +? > + drv->stats_size = resource_size(res); > + iounmap(offset_addr); > + > + drv->reg = devm_ioremap(&pdev->dev, drv->stats_base, drv->stats_size); > + if (!drv->reg) > + return -ENOMEM; > + > + if (create_debugfs_entries(drv)) > + return -ENODEV; > + > + platform_set_drvdata(pdev, drv); > + > + return 0; > +} ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver 2020-02-28 21:10 ` Stephen Boyd @ 2020-03-06 6:35 ` Maulik Shah 0 siblings, 0 replies; 15+ messages in thread From: Maulik Shah @ 2020-03-06 6:35 UTC (permalink / raw) To: Stephen Boyd, bjorn.andersson, evgreen, mka Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian On 2/29/2020 2:40 AM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-02-21 00:49:44) >> From: Mahesh Sivasubramanian <msivasub@codeaurora.org> >> >> Lets's add a driver to read the the stats from remote processor > Let's Done. >> and export to debugfs. >> >> The driver creates "soc_sleep_stats" directory in debugfs and adds >> files for various low power mode available. Below is sample output >> with command >> >> cat /sys/kernel/debug/soc_sleep_stats/ddr >> count = 0 >> Last Entered At = 0 >> Last Exited At = 0 >> Accumulated Duration = 0 >> >> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> drivers/soc/qcom/Kconfig | 10 ++ >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/soc_sleep_stats.c | 279 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 290 insertions(+) >> create mode 100644 drivers/soc/qcom/soc_sleep_stats.c >> >> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig >> index d0a73e7..4d36654 100644 >> --- a/drivers/soc/qcom/Kconfig >> +++ b/drivers/soc/qcom/Kconfig >> @@ -185,6 +185,16 @@ config QCOM_SOCINFO >> Say yes here to support the Qualcomm socinfo driver, providing >> information about the SoC to user space. >> >> +config QCOM_SOC_SLEEP_STATS >> + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver" >> + depends on ARCH_QCOM > Can you add || COMPILE_TEST? Done. >> + depends on DEBUG_FS >> + help >> + Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver to read >> + the shared memory exported by the remote processor related to >> + various SoC level low power modes statistics and export to debugfs >> + interface. >> + >> config QCOM_WCNSS_CTRL >> tristate "Qualcomm WCNSS control driver" >> depends on ARCH_QCOM || COMPILE_TEST >> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c >> new file mode 100644 >> index 00000000..bf38bb5 >> --- /dev/null >> +++ b/drivers/soc/qcom/soc_sleep_stats.c >> @@ -0,0 +1,279 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2011-2020, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/uaccess.h> > Why? will remove. >> +#include <linux/debugfs.h> >> +#include <linux/seq_file.h> >> +#include <linux/stddef.h> > Why? stddef.h was included for offsetof() usage. from below comments removed offsetof usage in entire driver and also this header. >> + >> +#include <linux/soc/qcom/smem.h> >> +#include <clocksource/arm_arch_timer.h> >> + >> +#define NAME_LENGTH 5 >> + >> +enum subsystem_item_id { >> + MODEM = 605, >> + ADSP, >> + CDSP, >> + SLPI, >> + GPU, >> + DISPLAY, >> +}; >> + >> +enum subsystem_pid { >> + PID_APSS = 0, >> + PID_MODEM = 1, >> + PID_ADSP = 2, >> + PID_SLPI = 3, >> + PID_CDSP = 5, >> + PID_GPU = PID_APSS, >> + PID_DISPLAY = PID_APSS, > I still don't understand this enums. Why not make them a #define set > with known values? Done. >> +}; >> + >> +struct subsystem_data { >> + char *name; > Can it be const char *? Done. >> + enum subsystem_item_id item_id; >> + enum subsystem_pid pid; >> +}; >> + >> +static struct subsystem_data subsystems[] = { >> + { "modem", MODEM, PID_MODEM }, >> + { "adsp", ADSP, PID_ADSP }, >> + { "cdsp", CDSP, PID_CDSP }, >> + { "slpi", SLPI, PID_SLPI }, >> + { "gpu", GPU, PID_GPU }, >> + { "display", DISPLAY, PID_DISPLAY }, >> +}; >> + >> +struct stats_config { >> + u32 offset_addr; > Looks ok but probably can just be unsigned int? Done. >> + u32 num_records; > Why not size_t? Is 32-bit width important? 32-bit width not important. >> + bool appended_stats_avail; >> +}; >> + >> +static const struct stats_config *config; > This shouldn't be necessary. Can the config be passed into the debugfs > file ops private data? No we are already passing address in debugfs file ops private data from where to start reading stats. will require to keep this global. >> + >> +struct soc_sleep_stats_data { > This struct is only used in probe. Why not just make probe have more > local variables? This is allocated in probe only struct soc_sleep_stats_data *drv; however we pass drv->reg + offset and drv->root in debugfs_create_file, these are used during show to indicate from where to start reading stats. also during remove, it currently only uses drv->root member to clean up debugfs, but lets pass entire drv, if there is need to undo something else in future. >> + phys_addr_t stats_base; >> + resource_size_t stats_size; >> + void __iomem *reg; >> + struct dentry *root; >> +}; >> + >> +struct entry { >> + __le32 stat_type; >> + __le32 count; >> + __le64 last_entered_at; >> + __le64 last_exited_at; >> + __le64 accumulated; > These should just be u32 and u64. Done. >> +}; >> + >> +struct appended_entry { >> + __le32 client_votes; >> + __le32 reserved[3]; >> +}; > Same, just u32. Done. >> + >> +static u64 get_time_in_sec(u64 counter) >> +{ >> + do_div(counter, arch_timer_get_rate()); > I don't think arch_timer_get_rate() is exported as a symbol. Why we need > to convert this to time? Is the counter not sufficient to understand > that so many ticks have passed since it entered and exited? Thanks, yes it is not exported. i think we can display without converting to seconds. if the need arises to display in seconds, we can add below locally #define ARCH_TIMER_FREQ 19200000 and then do_div with this value as done in v1. >> + >> + return counter; >> +} >> + >> +static void print_sleep_stats(struct seq_file *s, struct entry *e) >> +{ >> + e->last_entered_at = get_time_in_sec(e->last_entered_at); >> + e->last_exited_at = get_time_in_sec(e->last_exited_at); >> + e->accumulated = get_time_in_sec(e->accumulated); >> + >> + seq_printf(s, "count = %u\n", e->count); >> + seq_printf(s, "Last Entered At = %llu\n", e->last_entered_at); >> + seq_printf(s, "Last Exited At = %llu\n", e->last_exited_at); >> + seq_printf(s, "Accumulated Duration = %llu\n", e->accumulated); > Why is count not capitalized but everything else is? Done. >> +} >> + >> +static int subsystem_sleep_stats_show(struct seq_file *s, void *d) >> +{ >> + struct subsystem_data *subsystem = s->private; >> + struct entry *e; >> + >> + e = qcom_smem_get(subsystem->pid, subsystem->item_id, NULL); > Do we need to look this up each time we read the stats? Why can't we get > this once in probe or when we create the debugfs file? Yes, we need to look up each time in shared memory for latest values. >> + if (IS_ERR(e)) >> + return PTR_ERR(e); >> + >> + /* >> + * If a subsystem is in sleep when reading the sleep stats adjust >> + * the accumulated sleep duration to show actual sleep time. >> + */ >> + if (e->last_entered_at > e->last_exited_at) >> + e->accumulated += (arch_timer_read_counter() >> + - e->last_entered_at); >> + >> + print_sleep_stats(s, e); >> + >> + return 0; >> +} >> + >> +static int soc_sleep_stats_show(struct seq_file *s, void *d) >> +{ >> + void __iomem *reg = s->private; >> + uint32_t offset; >> + struct entry e; >> + >> + offset = offsetof(struct entry, count); >> + e.count = le32_to_cpu(readl_relaxed(reg + offset)); > readl APIs already do endian conversions. Done. >> + >> + offset = offsetof(struct entry, last_entered_at); >> + e.last_entered_at = le64_to_cpu(readq_relaxed(reg + offset)); >> + >> + offset = offsetof(struct entry, last_exited_at); >> + e.last_exited_at = le64_to_cpu(readq_relaxed(reg + offset)); >> + >> + offset = offsetof(struct entry, accumulated); >> + e.accumulated = le64_to_cpu(readq_relaxed(reg + offset)); >> + >> + print_sleep_stats(s, &e); >> + >> + if (config->appended_stats_avail) { >> + struct appended_entry ae; >> + >> + offset = offsetof(struct appended_entry, client_votes) + >> + sizeof(struct entry); >> + ae.client_votes = le32_to_cpu(readl_relaxed(reg + offset)); >> + seq_printf(s, "Client_votes = 0x%x\n", ae.client_votes); > Use %#x to avoid having to add 0x prefix. Also, can we replace '=' with > ':' so that readability is a bit nicer? Done. >> + } >> + >> + return 0; >> +} >> + >> +DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats); >> +DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats); >> + >> +static const struct stats_config rpm_data = { >> + .offset_addr = 0x14, >> + .num_records = 2, >> + .appended_stats_avail = true, >> +}; >> + >> +static const struct stats_config rpmh_data = { >> + .offset_addr = 0x4, >> + .num_records = 3, >> + .appended_stats_avail = false, >> +}; >> + >> +static const struct of_device_id soc_sleep_stats_table[] = { >> + { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data}, >> + { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data}, >> + { }, > Please drop comma after sentinel. It makes a compiler error appear if > anything comes after. Done. >> +}; > Move this table next to the driver structure please. Done. >> + >> +static int create_debugfs_entries(struct soc_sleep_stats_data *drv) >> +{ >> + struct entry *e; >> + char stat_type[NAME_LENGTH] = {0}; > Is it a string? Otherwise, seems pretty useless to initialize this to 0 > on the stack. yes its string. >> + uint32_t offset, type; > Just use u32 instead of uint32_t in any kernel code. Done >> + int i; >> + >> + drv->root = debugfs_create_dir("soc_sleep_stats", NULL); >> + if (IS_ERR_OR_NULL(drv->root)) >> + return PTR_ERR(drv->root); > This is wrong. Debugfs checks have generally been removed because it's > not a problem if debugfs fails. When it fails, drv->root will be NULL > and any debugfs_create() function will ignore it. Since this driver > depends on DEBUGFS being enabled, -ENODEV won't be returned as an error > pointer. Done. >> + >> + for (i = 0; i < config->num_records; i++) { >> + offset = offsetof(struct entry, stat_type) + >> + (i * sizeof(struct entry)); > This offset of stuff is sort of complicated. I'd prefer to treat it like > how we treat most registers with #defines and base + #define sort of > logic. That is easier to read. It also let's us decide to completely > reorder struct members and not have to worry that the struct doesn't > match how memory is laid out. Reordering members should not be done. qcom_smem_get() API returns pointer to "struct entry" type and will need keep order of members as it is. > Instead we have macros that define the > offset from some base __iomem pointer. Okay, i removed offsetof usage from entire driver and using #define now. >> + >> + if (config->appended_stats_avail) >> + offset += i * sizeof(struct appended_entry); >> + >> + type = le32_to_cpu(readl_relaxed(drv->reg + offset)); >> + memcpy(stat_type, &type, sizeof(uint32_t)); >> + debugfs_create_file(stat_type, 0444, drv->root, >> + drv->reg + offset, >> + &soc_sleep_stats_fops); >> + } >> + >> + for (i = 0; i < ARRAY_SIZE(subsystems); i++) { >> + e = qcom_smem_get(subsystems[i].pid, subsystems[i].item_id, >> + NULL); >> + >> + if (IS_ERR(e)) >> + continue; >> + >> + debugfs_create_file(subsystems[i].name, 0444, drv->root, >> + &subsystems[i], >> + &subsystem_sleep_stats_fops); >> + } >> + >> + return 0; >> +} >> + >> +static int soc_sleep_stats_probe(struct platform_device *pdev) >> +{ >> + struct soc_sleep_stats_data *drv; >> + struct resource *res; >> + void __iomem *offset_addr; >> + >> + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); >> + if (!drv) >> + return -ENOMEM; >> + >> + config = of_device_get_match_data(&pdev->dev); > Use device_get_match_data() to make this be less DT specific. Done. >> + if (!config) >> + return -ENODEV; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) >> + return PTR_ERR(res); >> + >> + offset_addr = ioremap_nocache(res->start + config->offset_addr, > Why do we need ioremap_nocache()? Can ioremap() work? Will update it to use devm_ioremap(). >> + sizeof(u32)); >> + if (IS_ERR(offset_addr)) >> + return PTR_ERR(offset_addr); >> + >> + drv->stats_base = res->start | readl_relaxed(offset_addr); > Why | vs. +? This is how final address need to be formed. Thanks, Maulik >> + drv->stats_size = resource_size(res); >> + iounmap(offset_addr); >> + >> + drv->reg = devm_ioremap(&pdev->dev, drv->stats_base, drv->stats_size); >> + if (!drv->reg) >> + return -ENOMEM; >> + >> + if (create_debugfs_entries(drv)) >> + return -ENODEV; >> + >> + platform_set_drvdata(pdev, drv); >> + >> + return 0; >> +} -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/4] arm64: dts: qcom: sc7180: Enable soc sleep stats 2020-02-21 8:49 [PATCH v2 0/4] Introduce SoC sleep stats driver Maulik Shah 2020-02-21 8:49 ` [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah 2020-02-21 8:49 ` [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver Maulik Shah @ 2020-02-21 8:49 ` Maulik Shah 2020-02-28 6:34 ` Bjorn Andersson 2020-02-21 8:49 ` [PATCH v2 4/4] arm64: defconfig: Enable SoC sleep stats driver for Qualcomm Maulik Shah 3 siblings, 1 reply; 15+ messages in thread From: Maulik Shah @ 2020-02-21 8:49 UTC (permalink / raw) To: swboyd, mka, evgreen, bjorn.andersson Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Maulik Shah, devicetree SoC sleep stats provides various low power mode stats. Cc: devicetree@vger.kernel.org Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index 8011c5f..eee6d92 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -745,6 +745,11 @@ }; }; + rpmh_sleep_stats: soc-sleep-stats@c3f0000 { + compatible = "qcom,rpmh-sleep-stats"; + reg = <0 0xc3f0000 0 0x400>; + }; + tcsr_mutex_regs: syscon@1f40000 { compatible = "syscon"; reg = <0 0x01f40000 0 0x40000>; -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] arm64: dts: qcom: sc7180: Enable soc sleep stats 2020-02-21 8:49 ` [PATCH v2 3/4] arm64: dts: qcom: sc7180: Enable soc sleep stats Maulik Shah @ 2020-02-28 6:34 ` Bjorn Andersson 2020-03-03 7:09 ` Maulik Shah 0 siblings, 1 reply; 15+ messages in thread From: Bjorn Andersson @ 2020-02-28 6:34 UTC (permalink / raw) To: Maulik Shah Cc: swboyd, mka, evgreen, linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, devicetree On Fri 21 Feb 00:49 PST 2020, Maulik Shah wrote: > SoC sleep stats provides various low power mode stats. > > Cc: devicetree@vger.kernel.org > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index 8011c5f..eee6d92 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -745,6 +745,11 @@ > }; > }; > > + rpmh_sleep_stats: soc-sleep-stats@c3f0000 { I don't see any reason to reference this node, so you should be able to omit the label(?) > + compatible = "qcom,rpmh-sleep-stats"; > + reg = <0 0xc3f0000 0 0x400>; Please pad the address to 8 digits and sort the nodes by address. Regards, Bjorn > + }; > + > tcsr_mutex_regs: syscon@1f40000 { > compatible = "syscon"; > reg = <0 0x01f40000 0 0x40000>; > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] arm64: dts: qcom: sc7180: Enable soc sleep stats 2020-02-28 6:34 ` Bjorn Andersson @ 2020-03-03 7:09 ` Maulik Shah 0 siblings, 0 replies; 15+ messages in thread From: Maulik Shah @ 2020-03-03 7:09 UTC (permalink / raw) To: Bjorn Andersson Cc: swboyd, mka, evgreen, linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, devicetree On 2/28/2020 12:04 PM, Bjorn Andersson wrote: > On Fri 21 Feb 00:49 PST 2020, Maulik Shah wrote: > >> SoC sleep stats provides various low power mode stats. >> >> Cc: devicetree@vger.kernel.org >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> index 8011c5f..eee6d92 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> @@ -745,6 +745,11 @@ >> }; >> }; >> >> + rpmh_sleep_stats: soc-sleep-stats@c3f0000 { > I don't see any reason to reference this node, so you should be able to > omit the label(?) Done. Will update in next revision. Thanks, Maulik > >> + compatible = "qcom,rpmh-sleep-stats"; >> + reg = <0 0xc3f0000 0 0x400>; > Please pad the address to 8 digits and sort the nodes by address. > > Regards, > Bjorn Done. Will update in next revision. Thanks, Maulik > >> + }; >> + >> tcsr_mutex_regs: syscon@1f40000 { >> compatible = "syscon"; >> reg = <0 0x01f40000 0 0x40000>; >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/4] arm64: defconfig: Enable SoC sleep stats driver for Qualcomm 2020-02-21 8:49 [PATCH v2 0/4] Introduce SoC sleep stats driver Maulik Shah ` (2 preceding siblings ...) 2020-02-21 8:49 ` [PATCH v2 3/4] arm64: dts: qcom: sc7180: Enable soc sleep stats Maulik Shah @ 2020-02-21 8:49 ` Maulik Shah 2020-02-28 6:37 ` Bjorn Andersson 3 siblings, 1 reply; 15+ messages in thread From: Maulik Shah @ 2020-02-21 8:49 UTC (permalink / raw) To: swboyd, mka, evgreen, bjorn.andersson Cc: linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao, Maulik Shah Enable SoC sleep stats driver. The driver gives statistics for various SoC level low power modes. Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 0f21288..c63399d 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -767,6 +767,7 @@ CONFIG_QCOM_SMD_RPM=y CONFIG_QCOM_SMP2P=y CONFIG_QCOM_SMSM=y CONFIG_QCOM_SOCINFO=m +CONFIG_QCOM_SOC_SLEEP_STATS=y CONFIG_ARCH_R8A774A1=y CONFIG_ARCH_R8A774B1=y CONFIG_ARCH_R8A774C0=y -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/4] arm64: defconfig: Enable SoC sleep stats driver for Qualcomm 2020-02-21 8:49 ` [PATCH v2 4/4] arm64: defconfig: Enable SoC sleep stats driver for Qualcomm Maulik Shah @ 2020-02-28 6:37 ` Bjorn Andersson 2020-03-03 7:08 ` Maulik Shah 0 siblings, 1 reply; 15+ messages in thread From: Bjorn Andersson @ 2020-02-28 6:37 UTC (permalink / raw) To: Maulik Shah Cc: swboyd, mka, evgreen, linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao On Fri 21 Feb 00:49 PST 2020, Maulik Shah wrote: > Enable SoC sleep stats driver. The driver gives statistics for > various SoC level low power modes. > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > arch/arm64/configs/defconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 0f21288..c63399d 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -767,6 +767,7 @@ CONFIG_QCOM_SMD_RPM=y > CONFIG_QCOM_SMP2P=y > CONFIG_QCOM_SMSM=y > CONFIG_QCOM_SOCINFO=m > +CONFIG_QCOM_SOC_SLEEP_STATS=y Afaict the driver is not crucial to boot the system or to collect the statics, so you should be able to make this =m. Regards, Bjorn > CONFIG_ARCH_R8A774A1=y > CONFIG_ARCH_R8A774B1=y > CONFIG_ARCH_R8A774C0=y > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/4] arm64: defconfig: Enable SoC sleep stats driver for Qualcomm 2020-02-28 6:37 ` Bjorn Andersson @ 2020-03-03 7:08 ` Maulik Shah 0 siblings, 0 replies; 15+ messages in thread From: Maulik Shah @ 2020-03-03 7:08 UTC (permalink / raw) To: Bjorn Andersson Cc: swboyd, mka, evgreen, linux-kernel, linux-arm-msm, agross, dianders, rnayak, ilina, lsrao On 2/28/2020 12:07 PM, Bjorn Andersson wrote: > On Fri 21 Feb 00:49 PST 2020, Maulik Shah wrote: > >> Enable SoC sleep stats driver. The driver gives statistics for >> various SoC level low power modes. >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> arch/arm64/configs/defconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >> index 0f21288..c63399d 100644 >> --- a/arch/arm64/configs/defconfig >> +++ b/arch/arm64/configs/defconfig >> @@ -767,6 +767,7 @@ CONFIG_QCOM_SMD_RPM=y >> CONFIG_QCOM_SMP2P=y >> CONFIG_QCOM_SMSM=y >> CONFIG_QCOM_SOCINFO=m >> +CONFIG_QCOM_SOC_SLEEP_STATS=y > Afaict the driver is not crucial to boot the system or to collect the > statics, so you should be able to make this =m. > > Regards, > Bjorn Done. Will update in next revision. Thanks, Maulik >> CONFIG_ARCH_R8A774A1=y >> CONFIG_ARCH_R8A774B1=y >> CONFIG_ARCH_R8A774C0=y >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-03-06 6:36 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-21 8:49 [PATCH v2 0/4] Introduce SoC sleep stats driver Maulik Shah 2020-02-21 8:49 ` [PATCH v2 1/4] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah 2020-02-26 20:05 ` Rob Herring 2020-02-28 6:38 ` Bjorn Andersson 2020-02-28 16:47 ` Stephen Boyd 2020-03-06 6:22 ` Maulik Shah 2020-02-21 8:49 ` [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver Maulik Shah 2020-02-28 21:10 ` Stephen Boyd 2020-03-06 6:35 ` Maulik Shah 2020-02-21 8:49 ` [PATCH v2 3/4] arm64: dts: qcom: sc7180: Enable soc sleep stats Maulik Shah 2020-02-28 6:34 ` Bjorn Andersson 2020-03-03 7:09 ` Maulik Shah 2020-02-21 8:49 ` [PATCH v2 4/4] arm64: defconfig: Enable SoC sleep stats driver for Qualcomm Maulik Shah 2020-02-28 6:37 ` Bjorn Andersson 2020-03-03 7:08 ` Maulik Shah
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).