Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 16 May 2023 14:58:51 -0700 | Subject | Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM | From | Trilok Soni <> |
| |
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
| ![\](/images/icornerr.gif) |