lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ticwyyycqlk2uqpiqckoqqnapqatw74s6f6tjqmmyt2d6siqqt@xxe2qdtr4c2c>
Date: Tue, 20 Aug 2024 13:20:33 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Gokul Sriram Palanisamy <quic_gokulsri@...cinc.com>,
	q@...k-bin.smtp.subspace.kernel.org
Cc: andersson@...nel.org, krzk+dt@...nel.org, 
	linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, quic_viswanat@...cinc.com, quic_mmanikan@...cinc.com, 
	quic_varada@...cinc.com, quic_srichara@...cinc.com
Subject: Re: [PATCH 1/2] dt-bindings: remoteproc: qcom: document hexagon
 based WCSS secure PIL

On Tue, Aug 20, 2024 at 02:25:14PM +0530, Gokul Sriram Palanisamy wrote:
> From: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> 
> Add new binding document for hexagon based WCSS secure PIL remoteproc.
> IPQ5332, IPQ9574 follows secure PIL remoteproc.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@...cinc.com>
> ---
>  .../remoteproc/qcom,wcss-sec-pil.yaml         | 125 ++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> new file mode 100644
> index 000000000000..c69401b6cec1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,wcss-sec-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm WCSS Secure Peripheral Image Loader

...

> +
> +maintainers:
> +  - Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> +
> +description:
> +  WCSS Secure Peripheral Image Loader loads firmware and power up QDSP6

What is WCSS? Don't add random acronyms without any explanation.

> +  remoteproc's on the Qualcomm IPQ9574, IPQ5332 SoC.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq5332-wcss-sec-pil
> +      - qcom,ipq9574-wcss-sec-pil
> +
> +  reg:
> +    maxItems: 1
> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Firmware name for the Hexagon core

No, look how other bindings are doing it.

It looks like you develop patches on some old kernel, so please start
from scratch on newest kernel.

> +
> +  interrupts:
> +    items:
> +      - description: Watchdog interrupt
> +      - description: Fatal interrupt
> +      - description: Ready interrupt
> +      - description: Handover interrupt
> +      - description: Stop acknowledge interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: wdog
> +      - const: fatal
> +      - const: ready
> +      - const: handover
> +      - const: stop-ack
> +
> +  clocks:
> +    items:
> +      - description: IM SLEEP clock

What is IM? Explain all acronyms.

What is SLEEP?

> +
> +  clock-names:
> +    items:
> +      - const: im_sleep

sleep? Are there different sleep clocks here?

> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the remote processor
> +    items:
> +      - description: Shutdown Q6
> +      - description: Stop Q6
> +

Do not introduce other order. First stop, then shutdown.

> +  qcom,smem-state-names:
> +    description:
> +      Names of the states used by the AP to signal the remote processor
> +    items:
> +      - const: shutdown
> +      - const: stop

The same.

> +
> +  memory-region:
> +    items:
> +      - description: Q6 reserved region
> +
> +  glink-edge:
> +    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
> +    description:
> +      Qualcomm G-Link subnode which represents communication edge, channels
> +      and devices related to the Modem.
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - firmware-name
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - qcom,smem-states
> +  - qcom,smem-state-names
> +  - memory-region

Keep the same order as in properties.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> +    q6v5_wcss: remoteproc@...0000 {

Drop unused label.

> +      compatible = "qcom,ipq5332-wcss-sec-pil";
> +      reg = <0xd100000 0x4040>;
> +      firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";
> +      interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
> +                            <&wcss_smp2p_in 0 IRQ_TYPE_NONE>,
> +                            <&wcss_smp2p_in 1 IRQ_TYPE_NONE>,

Best regards,
Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ