Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Dmitry Baryshkov]

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