On Tue, 16 May 2023 at 11:16, Arnd Bergmann <arnd@xxxxxxxx> wrote: > > On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote: > > On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury > > <quic_schowdhu@xxxxxxxxxxx> 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
