| Message ID | 343182748e12b6a4ac57d336405c50e36fc5520c.1683628357.git.quic_schowdhu@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | soc: qcom: boot_stats: Add driver support for boot_stats | expand |
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). Or we can follow the RPM master-stats approach, where the device is a top-level device, having handle pointers to the sram regions. > > Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > .../devicetree/bindings/sram/qcom,imem.yaml | 22 +++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml > index 0548e8e0d30b..bb884c5c8952 100644 > --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml > +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml > @@ -50,6 +50,28 @@ patternProperties: > $ref: /schemas/remoteproc/qcom,pil-info.yaml# > description: Peripheral image loader relocation region > > + "^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 > + > + required: > + - compatible > + - reg > + > required: > - compatible > - reg > -- > 2.17.1 >
On 5/9/2023 5:05 PM, 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). > Or we can follow the RPM master-stats approach, where the device is a > top-level device, having handle pointers to the sram regions. This is a dedicated region of IMEM reserved for storing stats related information. So it is represented as a child of IMEM, please refer to Documentation/devicetree/bindings/sram/sram.yaml which follows a similar philosophy. Also since this is a child of IMEM with a specific purpose, does it not warrant a dedicated driver? > >> >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> .../devicetree/bindings/sram/qcom,imem.yaml | 22 +++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml >> index 0548e8e0d30b..bb884c5c8952 100644 >> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml >> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml >> @@ -50,6 +50,28 @@ patternProperties: >> $ref: /schemas/remoteproc/qcom,pil-info.yaml# >> description: Peripheral image loader relocation region >> >> + "^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 >> + >> + required: >> + - compatible >> + - reg >> + >> required: >> - compatible >> - reg >> -- >> 2.17.1 >> > >
On Tue, 9 May 2023 at 15:21, Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote: > > > > On 5/9/2023 5:05 PM, 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). > > Or we can follow the RPM master-stats approach, where the device is a > > top-level device, having handle pointers to the sram regions. > > This is a dedicated region of IMEM reserved for storing stats related > information. So it is represented as a child of IMEM, please > refer to Documentation/devicetree/bindings/sram/sram.yaml which > follows a similar philosophy. Also since this is a child of IMEM with > a specific purpose, does it not warrant a dedicated driver? I do not question a dedicated driver. I was asking about the DT node. Even the mentioned bindings file describes the SRAM regions inside the SRAM, rather than a proper device to be instantiated in the SRAM node. I'd point to the boot_stats discussions (present on the list in the last several months). > > > > >> > >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> --- > >> .../devicetree/bindings/sram/qcom,imem.yaml | 22 +++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml > >> index 0548e8e0d30b..bb884c5c8952 100644 > >> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml > >> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml > >> @@ -50,6 +50,28 @@ patternProperties: > >> $ref: /schemas/remoteproc/qcom,pil-info.yaml# > >> description: Peripheral image loader relocation region > >> > >> + "^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 > >> + > >> + required: > >> + - compatible > >> + - reg > >> + > >> required: > >> - compatible > >> - reg > >> -- > >> 2.17.1 > >> > > > >
On 5/9/2023 6:35 PM, Dmitry Baryshkov wrote: > On Tue, 9 May 2023 at 15:21, Souradeep Chowdhury > <quic_schowdhu@quicinc.com> wrote: >> >> >> >> On 5/9/2023 5:05 PM, 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). >>> Or we can follow the RPM master-stats approach, where the device is a >>> top-level device, having handle pointers to the sram regions. >> >> This is a dedicated region of IMEM reserved for storing stats related >> information. So it is represented as a child of IMEM, please >> refer to Documentation/devicetree/bindings/sram/sram.yaml which >> follows a similar philosophy. Also since this is a child of IMEM with >> a specific purpose, does it not warrant a dedicated driver? > > I do not question a dedicated driver. I was asking about the DT node. > Even the mentioned bindings file describes the SRAM regions inside the > SRAM, rather than a proper device to be instantiated in the SRAM node. > I'd point to the boot_stats discussions (present on the list in the > last several months). > Ack. Will instantiate the device from the parent node in the driver and access the stats region to print the boot_stats information. >> >>> >>>> >>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> --- >>>> .../devicetree/bindings/sram/qcom,imem.yaml | 22 +++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml >>>> index 0548e8e0d30b..bb884c5c8952 100644 >>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml >>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml >>>> @@ -50,6 +50,28 @@ patternProperties: >>>> $ref: /schemas/remoteproc/qcom,pil-info.yaml# >>>> description: Peripheral image loader relocation region >>>> >>>> + "^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 >>>> + >>>> + required: >>>> + - compatible >>>> + - reg >>>> + >>>> required: >>>> - compatible >>>> - reg >>>> -- >>>> 2.17.1 >>>> >>> >>> > > >
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
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
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
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.
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
diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml index 0548e8e0d30b..bb884c5c8952 100644 --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml @@ -50,6 +50,28 @@ patternProperties: $ref: /schemas/remoteproc/qcom,pil-info.yaml# description: Peripheral image loader relocation region + "^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 + + required: + - compatible + - reg + required: - compatible - reg