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

[Trilok Soni]

On 5/16/2023 12:17 PM, Dmitry Baryshkov wrote:
> 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.

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