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

From: "Arnd Bergmann" <arnd@arndb.de>
To: "Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
	"Souradeep Chowdhury" <quic_schowdhu@quicinc.com>
Cc: "Andy Gross" <agross@kernel.org>,
	"Konrad Dybcio" <konrad.dybcio@somainline.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	devicetree@vger.kernel.org,
	"Sibi Sankar" <quic_sibis@quicinc.com>,
	"Rajendra Nayak" <quic_rjendra@quicinc.com>
Subject: Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
Date: Tue, 16 May 2023 10:16:15 +0200	[thread overview]
Message-ID: <7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com> (raw)
In-Reply-To: <CAA8EJpp2x2OEB2sg+caKmjkDYJp_NJ9mXo85FxTZr-9zRXHNhw@mail.gmail.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel