* Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM [not found] ` <CAA8EJpp2x2OEB2sg+caKmjkDYJp_NJ9mXo85FxTZr-9zRXHNhw@mail.gmail.com> @ 2023-05-16 8:16 ` Arnd Bergmann 2023-05-16 18:34 ` Trilok Soni 2023-05-16 19:17 ` Dmitry Baryshkov 0 siblings, 2 replies; 7+ messages in thread From: Arnd Bergmann @ 2023-05-16 8:16 UTC (permalink / raw) To: Dmitry Baryshkov, Souradeep Chowdhury Cc: Andy Gross, Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Rob Herring, linux-arm-kernel, linux-kernel, linux-arm-msm, devicetree, Sibi Sankar, Rajendra Nayak On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote: > On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury > <quic_schowdhu@quicinc.com> wrote: >> >> All Qualcomm bootloaders log useful timestamp information related >> to bootloader stats in the IMEM region. Add the child node within >> IMEM for the boot stat region containing register address and >> compatible string. > > I might have a minor vote here. Is there any reason why you have to > instantiate the device from DT? > It looks like a software interface. Ideally software should not be > described in DT (e.g. this can be instantiated from imem > driver-to-be). There is nothing wrong with describing firmware in DT, if that firmware is part of the platform, we do that for a lot of other bits of firmware. However, in this specific case, many things are wrong with the implementation, and neither the DT binding nor the driver makes sense to me in its current state. >> + "^stats@[0-9a-f]+$": >> + type: object >> + description: >> + Imem region dedicated for storing timestamps related >> + information regarding bootstats. >> + >> + additionalProperties: false >> + >> + properties: >> + compatible: >> + items: >> + - enum: >> + - qcom,sm8450-bootstats >> + - const: qcom,imem-bootstats >> + >> + reg: >> + maxItems: 1 If I understand this right, this "qcom,imem-bootstats" device serves as an indirection to store additional properties of the system in a memory area, but the description of that area is more complex than its contents, which makes no sense to me. Just create a binding for a firmware node in the devicetree itself, and put the values in properties of that. The first stage firmware can still use the same interface, but the actual loader that assembles the DT can get it out of that and store it in the properties. With that done, there is also no need for a kernel driver, as userspace can just get the values from /sys/firmware/devicetree/ directly. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM 2023-05-16 8:16 ` [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM Arnd Bergmann @ 2023-05-16 18:34 ` Trilok Soni 2023-05-16 18:41 ` Arnd Bergmann 2023-05-16 19:17 ` Dmitry Baryshkov 1 sibling, 1 reply; 7+ messages in thread From: Trilok Soni @ 2023-05-16 18:34 UTC (permalink / raw) To: Arnd Bergmann, Dmitry Baryshkov, Souradeep Chowdhury Cc: Andy Gross, Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Rob Herring, linux-arm-kernel, linux-kernel, linux-arm-msm, devicetree, Sibi Sankar, Rajendra Nayak On 5/16/2023 1:16 AM, Arnd Bergmann wrote: > On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote: >> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury >> <quic_schowdhu@quicinc.com> wrote: >>> >>> All Qualcomm bootloaders log useful timestamp information related >>> to bootloader stats in the IMEM region. Add the child node within >>> IMEM for the boot stat region containing register address and >>> compatible string. >> >> I might have a minor vote here. Is there any reason why you have to >> instantiate the device from DT? >> It looks like a software interface. Ideally software should not be >> described in DT (e.g. this can be instantiated from imem >> driver-to-be). > > There is nothing wrong with describing firmware in DT, if that > firmware is part of the platform, we do that for a lot of > other bits of firmware. > > However, in this specific case, many things are wrong with the > implementation, and neither the DT binding nor the driver > makes sense to me in its current state. > >>> + "^stats@[0-9a-f]+$": >>> + type: object >>> + description: >>> + Imem region dedicated for storing timestamps related >>> + information regarding bootstats. >>> + >>> + additionalProperties: false >>> + >>> + properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - qcom,sm8450-bootstats >>> + - const: qcom,imem-bootstats >>> + >>> + reg: >>> + maxItems: 1 > > If I understand this right, this "qcom,imem-bootstats" > device serves as an indirection to store additional > properties of the system in a memory area, but the description > of that area is more complex than its contents, which > makes no sense to me. > > Just create a binding for a firmware node in the devicetree > itself, and put the values in properties of that. The first > stage firmware can still use the same interface, but the > actual loader that assembles the DT can get it out of that > and store it in the properties. With that done, there is also > no need for a kernel driver, as userspace can just get the > values from /sys/firmware/devicetree/ directly. > To understand bit better here, what you are suggesting here that application bootloader (e.g., UEFI app for Linux) can read these boot values from the IMEM and encode them into the devicetree properties which can be later retrieved directly from the userspace. I am fine with this approach as well and in this case we just need to submit the bindings document, right? ---Trilok Soni _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM 2023-05-16 18:34 ` Trilok Soni @ 2023-05-16 18:41 ` Arnd Bergmann 0 siblings, 0 replies; 7+ messages in thread From: Arnd Bergmann @ 2023-05-16 18:41 UTC (permalink / raw) To: Trilok Soni, Dmitry Baryshkov, Souradeep Chowdhury Cc: Andy Gross, Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Rob Herring, linux-arm-kernel, linux-kernel, linux-arm-msm, devicetree, Sibi Sankar, Rajendra Nayak On Tue, May 16, 2023, at 20:34, Trilok Soni wrote: > On 5/16/2023 1:16 AM, Arnd Bergmann wrote: > > To understand bit better here, what you are suggesting here that > application bootloader (e.g., UEFI app for Linux) can read these > boot values from the IMEM and encode them into the devicetree properties > which can be later retrieved directly from the userspace. I'm not entirely sure I understand the concept of "application bootloader", but in principle this can be anything that runs before the devicetree is handed off from the final boot loader to the kernel. > I am fine with this approach as well and in this case we just > need to submit the bindings document, right? Yes, exactly. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM 2023-05-16 8:16 ` [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM Arnd Bergmann 2023-05-16 18:34 ` Trilok Soni @ 2023-05-16 19:17 ` Dmitry Baryshkov 2023-05-16 21:58 ` Trilok Soni 1 sibling, 1 reply; 7+ messages in thread From: Dmitry Baryshkov @ 2023-05-16 19:17 UTC (permalink / raw) To: Arnd Bergmann Cc: Souradeep Chowdhury, Andy Gross, Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Rob Herring, linux-arm-kernel, linux-kernel, linux-arm-msm, devicetree, Sibi Sankar, Rajendra Nayak On Tue, 16 May 2023 at 11:16, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote: > > On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury > > <quic_schowdhu@quicinc.com> wrote: > >> > >> All Qualcomm bootloaders log useful timestamp information related > >> to bootloader stats in the IMEM region. Add the child node within > >> IMEM for the boot stat region containing register address and > >> compatible string. > > > > I might have a minor vote here. Is there any reason why you have to > > instantiate the device from DT? > > It looks like a software interface. Ideally software should not be > > described in DT (e.g. this can be instantiated from imem > > driver-to-be). > > There is nothing wrong with describing firmware in DT, if that > firmware is part of the platform, we do that for a lot of > other bits of firmware. > > However, in this specific case, many things are wrong with the > implementation, and neither the DT binding nor the driver > makes sense to me in its current state. > > >> + "^stats@[0-9a-f]+$": > >> + type: object > >> + description: > >> + Imem region dedicated for storing timestamps related > >> + information regarding bootstats. > >> + > >> + additionalProperties: false > >> + > >> + properties: > >> + compatible: > >> + items: > >> + - enum: > >> + - qcom,sm8450-bootstats > >> + - const: qcom,imem-bootstats > >> + > >> + reg: > >> + maxItems: 1 > > If I understand this right, this "qcom,imem-bootstats" > device serves as an indirection to store additional > properties of the system in a memory area, but the description > of that area is more complex than its contents, which > makes no sense to me. > > Just create a binding for a firmware node in the devicetree > itself, and put the values in properties of that. The first > stage firmware can still use the same interface, but the > actual loader that assembles the DT can get it out of that > and store it in the properties. With that done, there is also > no need for a kernel driver, as userspace can just get the > values from /sys/firmware/devicetree/ directly. This sounds good, except the always-present issue of the devices which have already been released. Usually we can not expect a bootloader update for these devices. -- With best wishes Dmitry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM 2023-05-16 19:17 ` Dmitry Baryshkov @ 2023-05-16 21:58 ` Trilok Soni 0 siblings, 0 replies; 7+ messages in thread From: Trilok Soni @ 2023-05-16 21:58 UTC (permalink / raw) To: Dmitry Baryshkov, Arnd Bergmann Cc: Souradeep Chowdhury, Andy Gross, Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Rob Herring, linux-arm-kernel, linux-kernel, linux-arm-msm, devicetree, Sibi Sankar, Rajendra Nayak On 5/16/2023 12:17 PM, Dmitry Baryshkov wrote: > On Tue, 16 May 2023 at 11:16, Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote: >>> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury >>> <quic_schowdhu@quicinc.com> wrote: >>>> >>>> All Qualcomm bootloaders log useful timestamp information related >>>> to bootloader stats in the IMEM region. Add the child node within >>>> IMEM for the boot stat region containing register address and >>>> compatible string. >>> >>> I might have a minor vote here. Is there any reason why you have to >>> instantiate the device from DT? >>> It looks like a software interface. Ideally software should not be >>> described in DT (e.g. this can be instantiated from imem >>> driver-to-be). >> >> There is nothing wrong with describing firmware in DT, if that >> firmware is part of the platform, we do that for a lot of >> other bits of firmware. >> >> However, in this specific case, many things are wrong with the >> implementation, and neither the DT binding nor the driver >> makes sense to me in its current state. >> >>>> + "^stats@[0-9a-f]+$": >>>> + type: object >>>> + description: >>>> + Imem region dedicated for storing timestamps related >>>> + information regarding bootstats. >>>> + >>>> + additionalProperties: false >>>> + >>>> + properties: >>>> + compatible: >>>> + items: >>>> + - enum: >>>> + - qcom,sm8450-bootstats >>>> + - const: qcom,imem-bootstats >>>> + >>>> + reg: >>>> + maxItems: 1 >> >> If I understand this right, this "qcom,imem-bootstats" >> device serves as an indirection to store additional >> properties of the system in a memory area, but the description >> of that area is more complex than its contents, which >> makes no sense to me. >> >> Just create a binding for a firmware node in the devicetree >> itself, and put the values in properties of that. The first >> stage firmware can still use the same interface, but the >> actual loader that assembles the DT can get it out of that >> and store it in the properties. With that done, there is also >> no need for a kernel driver, as userspace can just get the >> values from /sys/firmware/devicetree/ directly. > > This sounds good, except the always-present issue of the devices which > have already been released. Usually we can not expect a bootloader > update for these devices. Valid point. I don't expect current SOCs supported at upstream to update with the newer bootloader having this feature done through bootloader. ---Trilok Soni _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <35863b47c04c2edd7ae49c57d23682aba6111d4f.1683628357.git.quic_schowdhu@quicinc.com>]
* Re: [PATCH V6 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats [not found] ` <35863b47c04c2edd7ae49c57d23682aba6111d4f.1683628357.git.quic_schowdhu@quicinc.com> @ 2023-05-16 8:19 ` Arnd Bergmann 2023-05-17 0:09 ` Dmitry Baryshkov 0 siblings, 1 reply; 7+ messages in thread From: Arnd Bergmann @ 2023-05-16 8:19 UTC (permalink / raw) To: Souradeep Chowdhury, Andy Gross, Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Rob Herring Cc: linux-arm-kernel, linux-kernel, linux-arm-msm, devicetree, Sibi Sankar, Rajendra Nayak On Tue, May 9, 2023, at 12:52, 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 show the information within debugfs file. > This information is useful in verifying if the existing boot KPIs have > regressed or not. The information is shown in milliseconds, a sample > log from sm8450(waipio) device is as follows:- > > /sys/kernel/debug/qcom_boot_stats # cat abl_time > 17898 ms > /sys/kernel/debug/qcom_boot_stats # cat pre_abl_time > 2879 ms > > 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". Documentation > details are also added in Documentation/ABI/testing/debugfs-driver-bootstat > > Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > --- > .../ABI/testing/debugfs-driver-bootstat | 17 +++ > drivers/soc/qcom/Kconfig | 10 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ As mentioned in my reply to the binding, I don't think this should be a driver at all. On top of that, even if it was a driver, it is clearly not a "soc" driver since nothing in it has any relevance to the hardware, rather than the first-stage loader, and drivers/soc/ drivers should never have their own user space interface either. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V6 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats 2023-05-16 8:19 ` [PATCH V6 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats Arnd Bergmann @ 2023-05-17 0:09 ` Dmitry Baryshkov 0 siblings, 0 replies; 7+ messages in thread From: Dmitry Baryshkov @ 2023-05-17 0:09 UTC (permalink / raw) To: Arnd Bergmann, Souradeep Chowdhury, Andy Gross, Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Rob Herring Cc: linux-arm-kernel, linux-kernel, linux-arm-msm, devicetree, Sibi Sankar, Rajendra Nayak On 16/05/2023 11:19, Arnd Bergmann wrote: > On Tue, May 9, 2023, at 12:52, 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 show the information within debugfs file. >> This information is useful in verifying if the existing boot KPIs have >> regressed or not. The information is shown in milliseconds, a sample >> log from sm8450(waipio) device is as follows:- >> >> /sys/kernel/debug/qcom_boot_stats # cat abl_time >> 17898 ms >> /sys/kernel/debug/qcom_boot_stats # cat pre_abl_time >> 2879 ms >> >> 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". Documentation >> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat >> >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> --- >> .../ABI/testing/debugfs-driver-bootstat | 17 +++ >> drivers/soc/qcom/Kconfig | 10 ++ >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ > > As mentioned in my reply to the binding, I don't think this should > be a driver at all. On top of that, even if it was a driver, it is > clearly not a "soc" driver since nothing in it has any relevance to > the hardware, rather than the first-stage loader, and drivers/soc/ > drivers should never have their own user space interface either. I suppose that we should add a proper driver for imem rather than always using it through syscon. -- With best wishes Dmitry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-17 0:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <cover.1683628357.git.quic_schowdhu@quicinc.com> [not found] ` <343182748e12b6a4ac57d336405c50e36fc5520c.1683628357.git.quic_schowdhu@quicinc.com> [not found] ` <CAA8EJpp2x2OEB2sg+caKmjkDYJp_NJ9mXo85FxTZr-9zRXHNhw@mail.gmail.com> 2023-05-16 8:16 ` [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM Arnd Bergmann 2023-05-16 18:34 ` Trilok Soni 2023-05-16 18:41 ` Arnd Bergmann 2023-05-16 19:17 ` Dmitry Baryshkov 2023-05-16 21:58 ` Trilok Soni [not found] ` <35863b47c04c2edd7ae49c57d23682aba6111d4f.1683628357.git.quic_schowdhu@quicinc.com> 2023-05-16 8:19 ` [PATCH V6 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats Arnd Bergmann 2023-05-17 0:09 ` Dmitry Baryshkov
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).