Received: by 2002:a05:6358:11c7:b0:104:8066:f915 with SMTP id i7csp1029072rwl; Fri, 7 Apr 2023 08:54:38 -0700 (PDT) X-Google-Smtp-Source: AKy350aa8CS15kGvHluDihZadPu3ox/l9OEKGJuWeT2sI6DBwa9KBwjY/zCx3U2fS4zCmfd74K0h X-Received: by 2002:a17:90b:1643:b0:23f:5c60:67b with SMTP id il3-20020a17090b164300b0023f5c60067bmr3121763pjb.5.1680882878446; Fri, 07 Apr 2023 08:54:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680882878; cv=none; d=google.com; s=arc-20160816; b=BL2ZZ+dwiFh3cjk2k32HwY7wHF4QF2WNpiaWBPfuCOHzm7in3j+KF7f7YqHVUgD+gO zMOVUEUgwFn9uXc0aTrJgpt+Zc50E2890LrY26Ebwu8rqt9xiALALKmKwYK8O4kVm50/ C4aN9jtBYLCfTG3t8nXt0maytVJa60YaPdpBEHgQqEQeP1z8fdl8XEBqTdg4GoRUYzD3 uGJqlzFlXrMunPgot6tmuw29G7Pg/UtiZgXmWMKcivJ/J/w5NYyO7FyodrLFiAgSwd4y U5x/sUOJUkkM4VSOF1nGHsoWNEFBW5oqu7X3Ss6QwuWgXkJNaeU15bvYQQyuRn2kAgjP Z3Iw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=hFe1553iajwBn4jp6HsXidBaWghKVfuECNO9PRhISek=; b=rpYdTnyjZzR/GKnC9zHkAUGMjxdfRGw3v1mto6LyXUFIBZRm+RV4+NURLueGbFByEC b3t+ujv+79lRJCFLwtBXe7QILkGTNKpH4YCHvleRe503ovqo5t9BsU2AyR9MM0htxthp TbGV3CZkvhSGnpZQ84sCYNgtmNIoJdtDkk3aP/ScXg19KtRisER8JelgWRyLL1zVb4Zl sNZexy5WAu8h1bpGdFUG73UVTvp5kK2zau7XutFosQYWQB1J4EOKMJgHptDAHPufwxaQ /AyW6Ie2FJKZ79H+r1k7C5FQ84ffIcHSWb6Gr64SwfcAcMjgOLANoZQZo8Xa52b02zdl qBcw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=SfUgfB5t; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id kr16-20020a170903081000b001a1ba07911bsi4004691plb.530.2023.04.07.08.54.22; Fri, 07 Apr 2023 08:54:38 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=SfUgfB5t; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233859AbjDGPiq (ORCPT + 99 others); Fri, 7 Apr 2023 11:38:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48224 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230140AbjDGPip (ORCPT ); Fri, 7 Apr 2023 11:38:45 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ACBCD8A73; Fri, 7 Apr 2023 08:38:43 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4873464BC8; Fri, 7 Apr 2023 15:38:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D739EC433EF; Fri, 7 Apr 2023 15:38:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1680881922; bh=kX8EO6zTETUVRE37+CMCYz+UhugNtYZVCG7LxiOVkPs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SfUgfB5t90kPbdSva6zhOWTwICyFqP+HQGXmFdZIoMTLKSVpo60AXtU9y0HweuzWm W/9r7Rhi3Gu/pH5RJNrdnef3/6XnxDgTozTH6XPoijaoeJqd3W27U4RaNZ2rtinvDr +/OMRTpDD6kXiTdXe9qmV7lTor6g7GvgN8V2VkdgqCdsMnsP/exkh2vwoJVuNNACqP VZcBUWeCzUizjaatMEfIIOBawxEWd/jqGPARHNgS5yyZVBZe0yG1c5Jbn8JEuyjrkD fGoM+4ZyMeXkrGtbOQasDzJkV6R6+dhAhMGNeA7SP2Fye4fTU7EqACJ35eH4mHOkIz W7Zx0mn8SwYMw== Date: Fri, 7 Apr 2023 08:41:32 -0700 From: Bjorn Andersson To: Souradeep Chowdhury Cc: Andy Gross , Konrad Dybcio , Krzysztof Kozlowski , Rob Herring , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, Sibi Sankar , Rajendra Nayak Subject: Re: [PATCH V2 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats Message-ID: <20230407154132.dpguz24f6rukyujq@ripper> References: <5eeeb46e9b3f61656a37cb77c2ad6a04e383c16d.1680874520.git.quic_schowdhu@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5eeeb46e9b3f61656a37cb77c2ad6a04e383c16d.1680874520.git.quic_schowdhu@quicinc.com> X-Spam-Status: No, score=-5.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 07, 2023 at 07:34:36PM +0530, Souradeep Chowdhury wrote: > All of Qualcomm's proprietary Android boot-loaders capture boot time > stats, like the time when the bootloader started execution and at what > point the bootloader handed over control to the kernel etc. in the IMEM > region. This information is captured in a specific format by this driver > by mapping a structure to the IMEM memory region and then accessing the > members of the structure to print the information. This information is > useful in verifying if the existing boot KPIs have regressed or not. > A sample log in SM8450(waipio) device is as follows:- > > KPI: Pre ABL Time = 3s > KPI: ABL Time = 14s Why are these in whole seconds? > KPI: Kernel MPM timestamp = 890206 And why is this presented in cycles? > > The Module Power Manager(MPM) sleep counter starts ticking at the PBL > stage and the timestamp generated by the sleep counter is logged by > the Qualcomm proprietary bootloader(ABL) at two points-> First when it > starts execution which is logged here as "Pre ABL Time" and the second > when it is about to load the kernel logged as "ABL Time". Both are > logged in the unit of seconds. We have a policy to not taint the kernel log with "useless" information, for kernel developers this seems to add no value and for end users there's no benefit to this. > The current kernel timestamp is > printed by the boot_stats driver as well. > Why? > Signed-off-by: Souradeep Chowdhury > --- > drivers/soc/qcom/Kconfig | 7 ++++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/boot_stats.c | 95 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 103 insertions(+) > create mode 100644 drivers/soc/qcom/boot_stats.c > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index d11bda2..2cfdbb7 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -79,6 +79,13 @@ config QCOM_DCC > driver provides interface to configure DCC block and read back > captured data from DCC's internal SRAM. > > +config QCOM_BOOTSTAT > + tristate "Qualcomm Technologies, Boot Stat driver" > + depends on ARCH_QCOM || COMPILE_TEST > + help > + This option enables driver for boot stats. Boot stat driver prints > + the kernel bootloader information by accessing the imem region. > + > config QCOM_KRYO_L2_ACCESSORS > bool > depends on ARCH_QCOM && ARM64 || COMPILE_TEST > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index 3b92c6c..8a9d995 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o > obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o > obj-$(CONFIG_QCOM_CPR) += cpr.o > obj-$(CONFIG_QCOM_DCC) += dcc.o > +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats_new.o Most other entries here are sorted alphabetically. > obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o > obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o > obj-$(CONFIG_QCOM_OCMEM) += ocmem.o > diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c > new file mode 100644 > index 0000000..080e820 > --- /dev/null > +++ b/drivers/soc/qcom/boot_stats.c > @@ -0,0 +1,95 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved. > + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define MPM_COUNTER_FREQ 32768 > + > +/** > + * struct boot_stats - timestamp information related to boot stats > + * @bootloader_start: Time for the starting point of the abl bootloader > + * @bootloader_end: Time when the kernel starts loading from abl bootloader > + */ > +struct boot_stats { > + u32 bootloader_start; > + u32 bootloader_end; bootloader != abl > +} __packed; > + > +struct boot_stats __iomem *boot_stats; > +void __iomem *mpm_counter_base; Why are these non-static global variables? > + > +static void print_boot_stats(void) > +{ > + u32 pre_abl_time = readl_relaxed(&boot_stats->bootloader_start) / MPM_COUNTER_FREQ; > + u32 abl_time = readl_relaxed(&boot_stats->bootloader_end) / MPM_COUNTER_FREQ; > + > + pr_info("KPI: Pre ABL Time = %us\n", pre_abl_time); > + pr_info("KPI: ABL Time = %us\n", abl_time); > + pr_info("KPI: Kernel MPM timestamp = %u\n", readl_relaxed(mpm_counter_base)); This number is completely dependent on link order and other things in happening in the kernel, so what trust do you give in this number going up or down? As above, why is this presented in ticks? > +} > + > +static int boot_stats_probe(struct platform_device *pdev) > +{ > + struct device_node *np_mpm2; > + struct device *boot_stat = &pdev->dev; > + > + boot_stats = of_iomap(boot_stat->of_node->child, 0); You can't just do ->child here, what if boot stats isn't the first item in the list? > + if (!boot_stats) > + return dev_err_probe(&pdev->dev, -ENOMEM, > + "failed to map imem region\n"); > + > + np_mpm2 = of_find_compatible_node(NULL, NULL, > + "qcom,mpm2-sleep-counter"); > + if (!np_mpm2) { > + return dev_err_probe(&pdev->dev, -EINVAL, > + "failed to get the counter node\n"); > + } > + > + if (of_get_address(np_mpm2, 0, NULL, NULL)) { > + mpm_counter_base = of_iomap(np_mpm2, 0); Isn't this region going to be also accessed by some sleep stats driver? > + if (!mpm_counter_base) { > + return dev_err_probe(&pdev->dev, -ENOMEM, > + "failed to map the counter\n"); > + } > + } > + print_boot_stats(); You're done with your platform_device, and your two ioremap regions here. But you're holding on to those 10kb of memory for the rest of the systems runtime. > + > + return 0; > +} > + > +static int boot_stats_remove(struct platform_device *pdev) > +{ > + iounmap(boot_stats); > + iounmap(mpm_counter_base); > + > + return 0; > +} > + > +static const struct of_device_id boot_stats_dt_match[] = { > + { .compatible = "qcom,sm8450-imem" }, You're binding to the root imem node, rather than your boot stats child node. How about just exposing the boot stats imem region to userspace, through debugfs or similar and then you can have a userspace tool that digs out and reports this information when profiling is relevant? Regards, Bjorn > + { } > +}; > +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); > + > +static struct platform_driver boot_stat_driver = { > + .probe = boot_stats_probe, > + .remove = boot_stats_remove, > + .driver = { > + .name = "qcom-boot-stats", > + .of_match_table = boot_stats_dt_match, > + }, > +}; > +module_platform_driver(boot_stat_driver); > + > +MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver"); > +MODULE_LICENSE("GPL"); > -- > 2.7.4 >