Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1687955688-20809-1-git-send-email-quic_mojha@quicinc.com> <1687955688-20809-9-git-send-email-quic_mojha@quicinc.com>
In-Reply-To: <1687955688-20809-9-git-send-email-quic_mojha@quicinc.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Wed, 28 Jun 2023 08:47:41 -0600
Message-ID: <CAL_JsqJ_TTnGjjB2d8_FKHpWBRG5GHLoWnabCKjsdeZ4QFdNEg@mail.gmail.com>
Subject: Re: [PATCH v4 08/21] dt-bindings: reserved-memory: Add qcom,ramoops binding
To:     Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     corbet@lwn.net, agross@kernel.org, andersson@kernel.org,
        konrad.dybcio@linaro.org, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, keescook@chromium.org, tony.luck@intel.com,
        gpiccoli@igalia.com, mathieu.poirier@linaro.org,
        catalin.marinas@arm.com, will@kernel.org, linus.walleij@linaro.org,
        andy.shevchenko@gmail.com, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-hardening@vger.kernel.org,
        linux-remoteproc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 28, 2023 at 6:36=E2=80=AFAM Mukesh Ojha <quic_mojha@quicinc.com=
> wrote:
>
> Qualcomm ramoops minidump logger provide a means of storing
> the ramoops data to some dynamically reserved memory instead
> of traditionally implemented ramoops where the region should
> be statically fixed ram region. Its device tree binding
> would be exactly same as ramoops device tree binding and is
> going to contain traditional ramoops frontend data and this
> content will be collected via Qualcomm minidump infrastructure
> provided from the boot firmware.

The big difference is if firmware is not deciding where this log
lives, then it doesn't need to be in DT. How does anything except the
kernel that allocates the log find the logs?

I'm pretty sure I already said all this before.

>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,ramoops.yaml | 126 +++++++++++++++=
++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ramoo=
ps.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,ramoops.yaml=
 b/Documentation/devicetree/bindings/soc/qcom/qcom,ramoops.yaml
> new file mode 100644
> index 000000000000..b1fdcf3f8ad4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,ramoops.yaml
> @@ -0,0 +1,126 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/soc/qcom/qcom,ramoops.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Qualcomm Ramoops minidump logger
> +
> +description: |
> +  Qualcomm ramoops minidump logger provide a means of storing the ramoop=
s
> +  data to some dynamically reserved memory instead of traditionally
> +  implemented ramoops where the region should be statically fixed ram
> +  region. Because of its similarity with ramoops it will also have same
> +  set of property what ramoops have it in its schema and is going to
> +  contain traditional ramoops frontend data and this region will be
> +  collected via Qualcomm minidump infrastructure provided from the
> +  boot firmware.
> +
> +maintainers:
> +  - Mukesh Ojha <quic_mojha@quicinc.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,sm8450-ramoops
> +      - const: qcom,ramoops
> +
> +  memory-region:
> +    maxItems: 1
> +    description: handle to memory reservation for qcom,ramoops region.
> +
> +  ecc-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: enables ECC support and specifies ECC buffer size in by=
tes
> +    default: 0 # no ECC
> +
> +  record-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: maximum size in bytes of each kmsg dump
> +    default: 0
> +
> +  console-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: size in bytes of log buffer reserved for kernel message=
s
> +    default: 0
> +
> +  ftrace-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: size in bytes of log buffer reserved for function traci=
ng and profiling
> +    default: 0
> +
> +  pmsg-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: size in bytes of log buffer reserved for userspace mess=
ages
> +    default: 0
> +
> +  mem-type:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: if present, sets the type of mapping is to be used to m=
ap the reserved region.
> +    default: 0
> +    oneOf:
> +      - const: 0
> +        description: write-combined
> +      - const: 1
> +        description: unbuffered
> +      - const: 2
> +        description: cached
> +
> +  max-reason:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 2 # log oopses and panics
> +    maximum: 0x7fffffff
> +    description: |
> +      If present, sets maximum type of kmsg dump reasons to store.
> +      This can be set to INT_MAX to store all kmsg dumps.
> +      See include/linux/kmsg_dump.h KMSG_DUMP_* for other kmsg dump reas=
on values.
> +      Setting this to 0 (KMSG_DUMP_UNDEF), means the reason filtering wi=
ll be
> +      controlled by the printk.always_kmsg_dump boot param.
> +      If unset, it will be 2 (KMSG_DUMP_OOPS), otherwise 5 (KMSG_DUMP_MA=
X).
> +
> +  flags:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0
> +    description: |
> +      If present, pass ramoops behavioral flags
> +      (see include/linux/pstore_ram.h RAMOOPS_FLAG_* for flag values).
> +
> +  no-dump-oops:
> +    deprecated: true

Why would you define deprecated properties in a *new* binding?

> +    type: boolean
> +    description: |
> +      Use max_reason instead. If present, and max_reason is not specifie=
d,
> +      it is equivalent to max_reason =3D 1 (KMSG_DUMP_PANIC).
> +
> +  unbuffered:
> +    deprecated: true
> +    type: boolean
> +    description: |
> +      Use mem_type instead. If present, and mem_type is not specified,
> +      it is equivalent to mem_type =3D 1 and uses unbuffered mappings to=
 map
> +      the reserved region (defaults to buffered mappings mem_type =3D 0)=
.
> +      If both are specified -- "mem_type" overrides "unbuffered".
> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - memory-region
> +
> +anyOf:
> +  - required: [record-size]
> +  - required: [console-size]
> +  - required: [ftrace-size]
> +  - required: [pmsg-size]
> +
> +examples:
> +  - |
> +
> +    qcom_ramoops {
> +        compatible =3D "qcom,sm8450-ramoops", "qcom,ramoops";
> +        memory-region =3D <&qcom_ramoops_region>;
> +        console-size =3D <0x8000>;    /* 32kB */
> +        record-size =3D <0x400>;      /*  1kB */
> +        ecc-size =3D <16>;
> +    };
> --
> 2.7.4
>