diff mbox series

[V2,01/13] dt-bindings: remoteproc: qcom: Add support for multipd model

Message ID 20230521222852.5740-2-quic_mmanikan@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add multipd remoteproc support | expand

Commit Message

Manikanta Mylavarapu May 21, 2023, 10:28 p.m. UTC
Add new binding document for multipd model remoteproc.
IPQ5018, IPQ9574 follows multipd model.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
---
Changes in V2:
	- Fixed all comments and rebased for TOT.
	- Changed to BSD-3-Clause based on internal open source team
          suggestion.
	- Added firmware-name.

 .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
 1 file changed, 265 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml

--
2.17.1

Comments

Krzysztof Kozlowski May 30, 2023, 10:46 a.m. UTC | #1
On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
> Add new binding document for multipd model remoteproc.
> IPQ5018, IPQ9574 follows multipd model.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> ---
> Changes in V2:
> 	- Fixed all comments and rebased for TOT.
> 	- Changed to BSD-3-Clause based on internal open source team
>           suggestion.
> 	- Added firmware-name.
> 
>  .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
>  1 file changed, 265 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> new file mode 100644
> index 000000000000..3257f27dc569
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> @@ -0,0 +1,265 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Multipd Secure Peripheral Image Loader
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
> +
> +description:
> +  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd
> +  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC. Pd means protection
> +  domain. It's similar to process in Linux. Here QDSP6 processor runs each
> +  wifi radio functionality on a separate process. One process can't access
> +  other process resources, so this is termed as PD i.e protection domain.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq5018-q6-mpd
> +      - qcom,ipq9574-q6-mpd
> +
> +  reg:
> +    maxItems: 1
> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description: Firmware name of the Hexagon core
> +
> +  interrupts-extended:
> +    items:
> +      - description: Watchdog interrupt
> +      - description: Fatal interrupt
> +      - description: Ready interrupt
> +      - description: Handover interrupt
> +      - description: Stop acknowledge interrupt


interrupts instead. The same in required:. I replied also to your
comment on your comment. :)

Best regards,
Krzysztof
Krzysztof Kozlowski May 30, 2023, 10:58 a.m. UTC | #2
On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
> Add new binding document for multipd model remoteproc.
> IPQ5018, IPQ9574 follows multipd model.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> ---
> Changes in V2:
> 	- Fixed all comments and rebased for TOT.
> 	- Changed to BSD-3-Clause based on internal open source team
>           suggestion.
> 	- Added firmware-name.
> 
>  .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
>  1 file changed, 265 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> new file mode 100644
> index 000000000000..3257f27dc569
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> @@ -0,0 +1,265 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Multipd Secure Peripheral Image Loader
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
> +
> +description:
> +  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd

... boots Q6 Protection Domain (PD), WCSS PD ...

> +  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC.

> Pd means protection
> +  domain. 

so you can skip this sentence as it is explained already.

> It's similar to process in Linux. Here QDSP6 processor runs each
> +  wifi radio functionality on a separate process. One process can't access
> +  other process resources, so this is termed as PD i.e protection domain.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq5018-q6-mpd
> +      - qcom,ipq9574-q6-mpd
> +
> +  reg:
> +    maxItems: 1
> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description: Firmware name of the Hexagon core

No need for ref and description. Instead maxItems.

> +
> +  interrupts-extended:
> +    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
> +
> +  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
> +
> +  qcom,smem-state-names:
> +    description:
> +      Names of the states used by the AP to signal the remote processor
> +    items:
> +      - const: shutdown
> +      - const: stop
> +
> +  memory-region:
> +    items:
> +      - description: Q6 pd 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.
> +
> +patternProperties:
> +  "^pd-1|pd-2|pd-3":
> +    type: object
> +    description:
> +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
> +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
> +      device node.

That's not enough. Your description does not say what is this, why you
have two protection domains for same compatible. What's more, it a bit
deviates from hardware description.

> +
> +    properties:
> +      compatible:
> +        enum:
> +          - qcom,ipq5018-wcss-ahb-mpd
> +          - qcom,ipq9574-wcss-ahb-mpd
> +          - qcom,ipq5018-wcss-pcie-mpd

Keep rather alphabetical order (so both 5018 together).

I also do not understand these at all. Why adding bus type to
compatible? This rarely is allowed (unless it is PCIe controller within
soc).

> +
> +      firmware-name:
> +        $ref: /schemas/types.yaml#/definitions/string-array
> +        items:
> +          - description: Firmware name of the Hexagon core

same comments

> +
> +      interrupts-extended:
> +        items:
> +          - description: Fatal interrupt
> +          - description: Ready interrupt
> +          - description: Spawn acknowledge interrupt
> +          - description: Stop acknowledge interrupt

ditto

> +
> +      interrupt-names:
> +        items:
> +          - const: fatal
> +          - const: ready
> +          - const: spawn-ack
> +          - const: stop-ack
> +
> +      qcom,smem-states:
> +        $ref: /schemas/types.yaml#/definitions/phandle-array
> +        description: States used by the AP to signal the remote processor
> +        items:
> +          - description: Shutdown WCSS pd
> +          - description: Stop WCSS pd
> +          - description: Spawn WCSS pd
> +
> +      qcom,smem-state-names:
> +        description:
> +          Names of the states used by the AP to signal the remote processor
> +        items:
> +          - const: shutdown
> +          - const: stop
> +          - const: spawn
> +
> +    required:
> +      - compatible
> +      - firmware-name
> +      - interrupts-extended
> +      - interrupt-names
> +      - qcom,smem-states
> +      - qcom,smem-state-names
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - firmware-name
> +  - reg
> +  - interrupts-extended
> +  - interrupt-names
> +  - qcom,smem-states
> +  - qcom,smem-state-names
> +  - memory-region
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq5018-q6-mpd
> +    then:
> +      properties:
> +        firmware-name:
> +          items:
> +            - const: IPQ5018/q6_fw.mdt
> +            - const: IPQ5018/m3_fw.mdt
> +            - const: qcn6122/m3_fw.mdt

No, names are not part of bindings. Also paths do not look correct. Open
linux-firmware package and verify these are good...

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq9574-q6-mpd
> +    then:
> +      properties:
> +        firmware-name:
> +          items:
> +            - const: IPQ9574/q6_fw.mdt
> +            - const: IPQ9574/m3_fw.mdt

Drop.

> +
> +unevaluatedProperties: false

This changed... why?


Best regards,
Krzysztof
Manikanta Mylavarapu June 5, 2023, 5:58 a.m. UTC | #3
On 5/30/2023 4:16 PM, Krzysztof Kozlowski wrote:
> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>> Add new binding document for multipd model remoteproc.
>> IPQ5018, IPQ9574 follows multipd model.
>>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> ---
>> Changes in V2:
>> 	- Fixed all comments and rebased for TOT.
>> 	- Changed to BSD-3-Clause based on internal open source team
>>            suggestion.
>> 	- Added firmware-name.
>>
>>   .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
>>   1 file changed, 265 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> new file mode 100644
>> index 000000000000..3257f27dc569
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> @@ -0,0 +1,265 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Multipd Secure Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
>> +
>> +description:
>> +  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd
>> +  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC. Pd means protection
>> +  domain. It's similar to process in Linux. Here QDSP6 processor runs each
>> +  wifi radio functionality on a separate process. One process can't access
>> +  other process resources, so this is termed as PD i.e protection domain.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq5018-q6-mpd
>> +      - qcom,ipq9574-q6-mpd
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  firmware-name:
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    description: Firmware name of the Hexagon core
>> +
>> +  interrupts-extended:
>> +    items:
>> +      - description: Watchdog interrupt
>> +      - description: Fatal interrupt
>> +      - description: Ready interrupt
>> +      - description: Handover interrupt
>> +      - description: Stop acknowledge interrupt
> 
> 
> interrupts instead. The same in required:. I replied also to your
> comment on your comment. :)
> 
> Best regards,
> Krzysztof
> 

I will use interrupts instead of interrupts-extended in dt-bindings.

Thanks & Regards,
Manikanta.
Manikanta Mylavarapu June 5, 2023, 8:03 a.m. UTC | #4
On 5/30/2023 4:28 PM, Krzysztof Kozlowski wrote:
> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>> Add new binding document for multipd model remoteproc.
>> IPQ5018, IPQ9574 follows multipd model.
>>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> ---
>> Changes in V2:
>> 	- Fixed all comments and rebased for TOT.
>> 	- Changed to BSD-3-Clause based on internal open source team
>>            suggestion.
>> 	- Added firmware-name.
>>
>>   .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
>>   1 file changed, 265 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> new file mode 100644
>> index 000000000000..3257f27dc569
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> @@ -0,0 +1,265 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Multipd Secure Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
>> +
>> +description:
>> +  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd
> 
> ... boots Q6 Protection Domain (PD), WCSS PD ...
> 
I will replace 'PD' with 'protection domain' wherever applicable.

>> +  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC.
> 
>> Pd means protection
>> +  domain.
> 
> so you can skip this sentence as it is explained already.
> 
Sure. I will skip.

>> It's similar to process in Linux. Here QDSP6 processor runs each
>> +  wifi radio functionality on a separate process. One process can't access
>> +  other process resources, so this is termed as PD i.e protection domain.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq5018-q6-mpd
>> +      - qcom,ipq9574-q6-mpd
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  firmware-name:
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    description: Firmware name of the Hexagon core
> 
> No need for ref and description. Instead maxItems.
> 
Ok. I will remove ref, description and add maxItems.

>> +
>> +  interrupts-extended:
>> +    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
>> +
>> +  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
>> +
>> +  qcom,smem-state-names:
>> +    description:
>> +      Names of the states used by the AP to signal the remote processor
>> +    items:
>> +      - const: shutdown
>> +      - const: stop
>> +
>> +  memory-region:
>> +    items:
>> +      - description: Q6 pd 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.
>> +
>> +patternProperties:
>> +  "^pd-1|pd-2|pd-3":
>> +    type: object
>> +    description:
>> +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
>> +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
>> +      device node.
> 
> That's not enough. Your description does not say what is this, why you
> have two protection domains for same compatible. What's more, it a bit
> deviates from hardware description.
> 
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - qcom,ipq5018-wcss-ahb-mpd
>> +          - qcom,ipq9574-wcss-ahb-mpd
>> +          - qcom,ipq5018-wcss-pcie-mpd
> 
> Keep rather alphabetical order (so both 5018 together).
> 
Sure, i will update.

> I also do not understand these at all. Why adding bus type to
> compatible? This rarely is allowed (unless it is PCIe controller within
> soc).
> 
>> +
>> +      firmware-name:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        items:
>> +          - description: Firmware name of the Hexagon core
> 
> same comments
> 
Ok. I will remove ref, description and add maxItems.

>> +
>> +      interrupts-extended:
>> +        items:
>> +          - description: Fatal interrupt
>> +          - description: Ready interrupt
>> +          - description: Spawn acknowledge interrupt
>> +          - description: Stop acknowledge interrupt
> 
> ditto
> 
I will use 'interrupts' here.

Thanks & Regards,
Manikanta.

>> +
>> +      interrupt-names:
>> +        items:
>> +          - const: fatal
>> +          - const: ready
>> +          - const: spawn-ack
>> +          - const: stop-ack
>> +
>> +      qcom,smem-states:
>> +        $ref: /schemas/types.yaml#/definitions/phandle-array
>> +        description: States used by the AP to signal the remote processor
>> +        items:
>> +          - description: Shutdown WCSS pd
>> +          - description: Stop WCSS pd
>> +          - description: Spawn WCSS pd
>> +
>> +      qcom,smem-state-names:
>> +        description:
>> +          Names of the states used by the AP to signal the remote processor
>> +        items:
>> +          - const: shutdown
>> +          - const: stop
>> +          - const: spawn
>> +
>> +    required:
>> +      - compatible
>> +      - firmware-name
>> +      - interrupts-extended
>> +      - interrupt-names
>> +      - qcom,smem-states
>> +      - qcom,smem-state-names
>> +
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - firmware-name
>> +  - reg
>> +  - interrupts-extended
>> +  - interrupt-names
>> +  - qcom,smem-states
>> +  - qcom,smem-state-names
>> +  - memory-region
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq5018-q6-mpd
>> +    then:
>> +      properties:
>> +        firmware-name:
>> +          items:
>> +            - const: IPQ5018/q6_fw.mdt
>> +            - const: IPQ5018/m3_fw.mdt
>> +            - const: qcn6122/m3_fw.mdt
> 
> No, names are not part of bindings. Also paths do not look correct. Open
> linux-firmware package and verify these are good...
> 
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq9574-q6-mpd
>> +    then:
>> +      properties:
>> +        firmware-name:
>> +          items:
>> +            - const: IPQ9574/q6_fw.mdt
>> +            - const: IPQ9574/m3_fw.mdt
> 
> Drop.
> 
>> +
>> +unevaluatedProperties: false
> 
> This changed... why?
> 
> 
> Best regards,
> Krzysztof
>
Manikanta Mylavarapu June 5, 2023, 12:02 p.m. UTC | #5
On 5/30/2023 4:28 PM, Krzysztof Kozlowski wrote:
> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>> Add new binding document for multipd model remoteproc.
>> IPQ5018, IPQ9574 follows multipd model.
>>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> ---
>> Changes in V2:
>> 	- Fixed all comments and rebased for TOT.
>> 	- Changed to BSD-3-Clause based on internal open source team
>>            suggestion.
>> 	- Added firmware-name.
>>
>>   .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
>>   1 file changed, 265 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> new file mode 100644
>> index 000000000000..3257f27dc569
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> @@ -0,0 +1,265 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Multipd Secure Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
>> +
>> +description:
>> +  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd
> 
> ... boots Q6 Protection Domain (PD), WCSS PD ...
> 
>> +  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC.
> 
>> Pd means protection
>> +  domain.
> 
> so you can skip this sentence as it is explained already.
> 
>> It's similar to process in Linux. Here QDSP6 processor runs each
>> +  wifi radio functionality on a separate process. One process can't access
>> +  other process resources, so this is termed as PD i.e protection domain.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq5018-q6-mpd
>> +      - qcom,ipq9574-q6-mpd
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  firmware-name:
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    description: Firmware name of the Hexagon core
> 
> No need for ref and description. Instead maxItems.
> 
>> +
>> +  interrupts-extended:
>> +    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
>> +
>> +  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
>> +
>> +  qcom,smem-state-names:
>> +    description:
>> +      Names of the states used by the AP to signal the remote processor
>> +    items:
>> +      - const: shutdown
>> +      - const: stop
>> +
>> +  memory-region:
>> +    items:
>> +      - description: Q6 pd 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.
>> +
>> +patternProperties:
>> +  "^pd-1|pd-2|pd-3":
>> +    type: object
>> +    description:
>> +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
>> +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
>> +      device node.
> 
> That's not enough. Your description does not say what is this, why you
> have two protection domains for same compatible. What's more, it a bit
> deviates from hardware description.
>
WCSS means 'wireless connectivity sub system', in simple words it's a
wifi radio block.

IPQ5018 SOC has both internal (AHB) wifi radio/WCSS and external (PCIE)
wifi radio/WCSS. In Q6, Root protection domain will provide services to
both internal (AHB) and external (PCIE) wifi radio's protection domain.
So we have two protection domains for IPQ5018, one is for internal(AHB) 
and other is for external(PCIE) wifi radio.

>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - qcom,ipq5018-wcss-ahb-mpd
>> +          - qcom,ipq9574-wcss-ahb-mpd
>> +          - qcom,ipq5018-wcss-pcie-mpd
> 
> Keep rather alphabetical order (so both 5018 together).
> 
> I also do not understand these at all. Why adding bus type to
> compatible? This rarely is allowed (unless it is PCIe controller within
> soc).
> 
IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE 
radio's properties, i have added bus type to compatible.

>> +
>> +      firmware-name:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        items:
>> +          - description: Firmware name of the Hexagon core
> 
> same comments
> 
>> +
>> +      interrupts-extended:
>> +        items:
>> +          - description: Fatal interrupt
>> +          - description: Ready interrupt
>> +          - description: Spawn acknowledge interrupt
>> +          - description: Stop acknowledge interrupt
> 
> ditto
> 
>> +
>> +      interrupt-names:
>> +        items:
>> +          - const: fatal
>> +          - const: ready
>> +          - const: spawn-ack
>> +          - const: stop-ack
>> +
>> +      qcom,smem-states:
>> +        $ref: /schemas/types.yaml#/definitions/phandle-array
>> +        description: States used by the AP to signal the remote processor
>> +        items:
>> +          - description: Shutdown WCSS pd
>> +          - description: Stop WCSS pd
>> +          - description: Spawn WCSS pd
>> +
>> +      qcom,smem-state-names:
>> +        description:
>> +          Names of the states used by the AP to signal the remote processor
>> +        items:
>> +          - const: shutdown
>> +          - const: stop
>> +          - const: spawn
>> +
>> +    required:
>> +      - compatible
>> +      - firmware-name
>> +      - interrupts-extended
>> +      - interrupt-names
>> +      - qcom,smem-states
>> +      - qcom,smem-state-names
>> +
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - firmware-name
>> +  - reg
>> +  - interrupts-extended
>> +  - interrupt-names
>> +  - qcom,smem-states
>> +  - qcom,smem-state-names
>> +  - memory-region
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq5018-q6-mpd
>> +    then:
>> +      properties:
>> +        firmware-name:
>> +          items:
>> +            - const: IPQ5018/q6_fw.mdt
>> +            - const: IPQ5018/m3_fw.mdt
>> +            - const: qcn6122/m3_fw.mdt
> 
> No, names are not part of bindings. Also paths do not look correct. Open
> linux-firmware package and verify these are good...
> 
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq9574-q6-mpd
>> +    then:
>> +      properties:
>> +        firmware-name:
>> +          items:
>> +            - const: IPQ9574/q6_fw.mdt
>> +            - const: IPQ9574/m3_fw.mdt
> 
> Drop.
> 
>> +
>> +unevaluatedProperties: false
> 
> This changed... why?
> 
> 
'unevaluatedProperties' is similar to 'additionalProperties' except
that it recognize properties declared in subschemas as well.

Thanks & Regards,
Manikanta.

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski June 6, 2023, 6:14 a.m. UTC | #6
On 05/06/2023 14:02, Manikanta Mylavarapu wrote:
>>> +  memory-region:
>>> +    items:
>>> +      - description: Q6 pd 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.
>>> +
>>> +patternProperties:
>>> +  "^pd-1|pd-2|pd-3":
>>> +    type: object
>>> +    description:
>>> +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
>>> +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
>>> +      device node.
>>
>> That's not enough. Your description does not say what is this, why you
>> have two protection domains for same compatible. What's more, it a bit
>> deviates from hardware description.
>>
> WCSS means 'wireless connectivity sub system', in simple words it's a
> wifi radio block.
> 
> IPQ5018 SOC has both internal (AHB) wifi radio/WCSS and external (PCIE)
> wifi radio/WCSS. In Q6, Root protection domain will provide services to
> both internal (AHB) and external (PCIE) wifi radio's protection domain.
> So we have two protection domains for IPQ5018, one is for internal(AHB) 
> and other is for external(PCIE) wifi radio.

So it is now in email, but not in the code...
> 
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        enum:
>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>
>> Keep rather alphabetical order (so both 5018 together).
>>
>> I also do not understand these at all. Why adding bus type to
>> compatible? This rarely is allowed (unless it is PCIe controller within
>> soc).
>>
> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE 
> radio's properties, i have added bus type to compatible.

It's the same device - WCSS - right? We do not create multiple nodes and
compatibles for the same devices. Bus suffixes are almost never parts of
compatibles.


>>
>> Drop.
>>
>>> +
>>> +unevaluatedProperties: false
>>
>> This changed... why?
>>
>>
> 'unevaluatedProperties' is similar to 'additionalProperties' except
> that it recognize properties declared in subschemas as well.

You don't have to explain me what are unevaluatedProperties or
additionalProperties. Let's assume that I know them. What you should
explain is why you changed it. Where is the reference to other schema?


Best regards,
Krzysztof
Manikanta Mylavarapu June 6, 2023, 12:11 p.m. UTC | #7
On 6/6/2023 11:44 AM, Krzysztof Kozlowski wrote:
> On 05/06/2023 14:02, Manikanta Mylavarapu wrote:
>>>> +  memory-region:
>>>> +    items:
>>>> +      - description: Q6 pd 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.
>>>> +
>>>> +patternProperties:
>>>> +  "^pd-1|pd-2|pd-3":
>>>> +    type: object
>>>> +    description:
>>>> +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
>>>> +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
>>>> +      device node.
>>>
>>> That's not enough. Your description does not say what is this, why you
>>> have two protection domains for same compatible. What's more, it a bit
>>> deviates from hardware description.
>>>
>> WCSS means 'wireless connectivity sub system', in simple words it's a
>> wifi radio block.
>>
>> IPQ5018 SOC has both internal (AHB) wifi radio/WCSS and external (PCIE)
>> wifi radio/WCSS. In Q6, Root protection domain will provide services to
>> both internal (AHB) and external (PCIE) wifi radio's protection domain.
>> So we have two protection domains for IPQ5018, one is for internal(AHB)
>> and other is for external(PCIE) wifi radio.
> 
> So it is now in email, but not in the code...
>>
I will add this info, corresponding block diagram in driver code.

>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        enum:
>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>
>>> Keep rather alphabetical order (so both 5018 together).
>>>
>>> I also do not understand these at all. Why adding bus type to
>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>> soc).
>>>
>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>> radio's properties, i have added bus type to compatible.
> 
> It's the same device - WCSS - right? We do not create multiple nodes and
> compatibles for the same devices. Bus suffixes are almost never parts of
> compatibles.
> 
> 
No it's not the same device. WCSS on inside IPQ5018 and WCSS attached 
via pcie to IPQ5018. Here QDSP6 managing both WCSS's.

So for better clarity i will use attached SOC ID in compatible.
Below are the new compatible's.

- qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
- qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
- qcom,qcn6122-wcss-mpd //IPQ5018 attached radio

>>>
>>> Drop.
>>>
>>>> +
>>>> +unevaluatedProperties: false
>>>
>>> This changed... why?
>>>
>>>
>> 'unevaluatedProperties' is similar to 'additionalProperties' except
>> that it recognize properties declared in subschemas as well.
> 
> You don't have to explain me what are unevaluatedProperties or
> additionalProperties. Let's assume that I know them. What you should
> explain is why you changed it. Where is the reference to other schema?
> 
I will go with previously used 'additionalProperties' itself and add
'unevaluatedProperties' in glink-edge.

Thanks & Regards,
Manikanta.
> 
> Best regards,
> Krzysztof
>
Kalle Valo June 6, 2023, 1:49 p.m. UTC | #8
Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes:

>>>>> +
>>>>> +    properties:
>>>>> +      compatible:
>>>>> +        enum:
>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>
>>>> Keep rather alphabetical order (so both 5018 together).
>>>>
>>>> I also do not understand these at all. Why adding bus type to
>>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>>> soc).
>>>>
>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>>> radio's properties, i have added bus type to compatible.
>>
>> It's the same device - WCSS - right? We do not create multiple nodes and
>> compatibles for the same devices. Bus suffixes are almost never parts of
>> compatibles.
>
>
> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>
> So for better clarity i will use attached SOC ID in compatible.
> Below are the new compatible's.
>
> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
> - qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio

What mandates that there's just one QCN6122 device attached to PCI?
Assuming fixed PCI configurations like that makes me worried.
Manikanta Mylavarapu June 7, 2023, 8:10 a.m. UTC | #9
On 6/6/2023 7:19 PM, Kalle Valo wrote:
> Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes:
> 
>>>>>> +
>>>>>> +    properties:
>>>>>> +      compatible:
>>>>>> +        enum:
>>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>>
>>>>> Keep rather alphabetical order (so both 5018 together).
>>>>>
>>>>> I also do not understand these at all. Why adding bus type to
>>>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>>>> soc).
>>>>>
>>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>>>> radio's properties, i have added bus type to compatible.
>>>
>>> It's the same device - WCSS - right? We do not create multiple nodes and
>>> compatibles for the same devices. Bus suffixes are almost never parts of
>>> compatibles.
>>
>>
>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>>
>> So for better clarity i will use attached SOC ID in compatible.
>> Below are the new compatible's.
>>
>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
>> - qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio
> 
> What mandates that there's just one QCN6122 device attached to PCI?
> Assuming fixed PCI configurations like that makes me worried.
> 

IPQ5018 always has one internal radio, attached pcie radio's depends on 
no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two 
qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's 
number of pcie devices controlled by QDSP6.

Thanks & Regards,
Manikanta.
Krzysztof Kozlowski June 7, 2023, 8:27 a.m. UTC | #10
On 07/06/2023 10:10, Manikanta Mylavarapu wrote:
> 
> 
> On 6/6/2023 7:19 PM, Kalle Valo wrote:
>> Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes:
>>
>>>>>>> +
>>>>>>> +    properties:
>>>>>>> +      compatible:
>>>>>>> +        enum:
>>>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>>>
>>>>>> Keep rather alphabetical order (so both 5018 together).
>>>>>>
>>>>>> I also do not understand these at all. Why adding bus type to
>>>>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>>>>> soc).
>>>>>>
>>>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>>>>> radio's properties, i have added bus type to compatible.
>>>>
>>>> It's the same device - WCSS - right? We do not create multiple nodes and
>>>> compatibles for the same devices. Bus suffixes are almost never parts of
>>>> compatibles.
>>>
>>>
>>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
>>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>>>
>>> So for better clarity i will use attached SOC ID in compatible.
>>> Below are the new compatible's.
>>>
>>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
>>> - qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
>>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio
>>
>> What mandates that there's just one QCN6122 device attached to PCI?
>> Assuming fixed PCI configurations like that makes me worried.
>>
> 
> IPQ5018 always has one internal radio, attached pcie radio's depends on 
> no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two 
> qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's 
> number of pcie devices controlled by QDSP6.

So this is hot-pluggable (or at least board-pluggable), then should not
be a part of static DTS.

Some concepts of virtual-processes is anyway far away from hardware
description, thus does not fit into DTS. Adding now to the equation PCIe
with variable number of such processes, brings us even further.

This is not a DT property. Remember - DT describes hardware.

Best regards,
Krzysztof
Manikanta Mylavarapu June 8, 2023, 12:59 p.m. UTC | #11
On 5/30/2023 4:28 PM, Krzysztof Kozlowski wrote:
> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>> Add new binding document for multipd model remoteproc.
>> IPQ5018, IPQ9574 follows multipd model.
>>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> ---
>> Changes in V2:
>> 	- Fixed all comments and rebased for TOT.
>> 	- Changed to BSD-3-Clause based on internal open source team
>>            suggestion.
>> 	- Added firmware-name.
>>
>>   .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
>>   1 file changed, 265 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> new file mode 100644
>> index 000000000000..3257f27dc569
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
>> @@ -0,0 +1,265 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Multipd Secure Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
>> +
>> +description:
>> +  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd
> 
> ... boots Q6 Protection Domain (PD), WCSS PD ...
> 
>> +  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC.
> 
>> Pd means protection
>> +  domain.
> 
> so you can skip this sentence as it is explained already.
> 
>> It's similar to process in Linux. Here QDSP6 processor runs each
>> +  wifi radio functionality on a separate process. One process can't access
>> +  other process resources, so this is termed as PD i.e protection domain.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq5018-q6-mpd
>> +      - qcom,ipq9574-q6-mpd
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  firmware-name:
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    description: Firmware name of the Hexagon core
> 
> No need for ref and description. Instead maxItems.
> 
>> +
>> +  interrupts-extended:
>> +    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
>> +
>> +  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
>> +
>> +  qcom,smem-state-names:
>> +    description:
>> +      Names of the states used by the AP to signal the remote processor
>> +    items:
>> +      - const: shutdown
>> +      - const: stop
>> +
>> +  memory-region:
>> +    items:
>> +      - description: Q6 pd 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.
>> +
>> +patternProperties:
>> +  "^pd-1|pd-2|pd-3":
>> +    type: object
>> +    description:
>> +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
>> +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
>> +      device node.
> 
> That's not enough. Your description does not say what is this, why you
> have two protection domains for same compatible. What's more, it a bit
> deviates from hardware description.
> 
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - qcom,ipq5018-wcss-ahb-mpd
>> +          - qcom,ipq9574-wcss-ahb-mpd
>> +          - qcom,ipq5018-wcss-pcie-mpd
> 
> Keep rather alphabetical order (so both 5018 together).
> 
> I also do not understand these at all. Why adding bus type to
> compatible? This rarely is allowed (unless it is PCIe controller within
> soc).
> 
>> +
>> +      firmware-name:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        items:
>> +          - description: Firmware name of the Hexagon core
> 
> same comments
> 
>> +
>> +      interrupts-extended:
>> +        items:
>> +          - description: Fatal interrupt
>> +          - description: Ready interrupt
>> +          - description: Spawn acknowledge interrupt
>> +          - description: Stop acknowledge interrupt
> 
> ditto
> 
>> +
>> +      interrupt-names:
>> +        items:
>> +          - const: fatal
>> +          - const: ready
>> +          - const: spawn-ack
>> +          - const: stop-ack
>> +
>> +      qcom,smem-states:
>> +        $ref: /schemas/types.yaml#/definitions/phandle-array
>> +        description: States used by the AP to signal the remote processor
>> +        items:
>> +          - description: Shutdown WCSS pd
>> +          - description: Stop WCSS pd
>> +          - description: Spawn WCSS pd
>> +
>> +      qcom,smem-state-names:
>> +        description:
>> +          Names of the states used by the AP to signal the remote processor
>> +        items:
>> +          - const: shutdown
>> +          - const: stop
>> +          - const: spawn
>> +
>> +    required:
>> +      - compatible
>> +      - firmware-name
>> +      - interrupts-extended
>> +      - interrupt-names
>> +      - qcom,smem-states
>> +      - qcom,smem-state-names
>> +
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - firmware-name
>> +  - reg
>> +  - interrupts-extended
>> +  - interrupt-names
>> +  - qcom,smem-states
>> +  - qcom,smem-state-names
>> +  - memory-region
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq5018-q6-mpd
>> +    then:
>> +      properties:
>> +        firmware-name:
>> +          items:
>> +            - const: IPQ5018/q6_fw.mdt
>> +            - const: IPQ5018/m3_fw.mdt
>> +            - const: qcn6122/m3_fw.mdt
> 
> No, names are not part of bindings. Also paths do not look correct. Open
> linux-firmware package and verify these are good...
> 
I will remove names from bindings and use firmware path as per
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/ath11k/IPQ5018/hw1.0

>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq9574-q6-mpd
>> +    then:
>> +      properties:
>> +        firmware-name:
>> +          items:
>> +            - const: IPQ9574/q6_fw.mdt
>> +            - const: IPQ9574/m3_fw.mdt
> 
> Drop.
> 
I will drop.

Thanks & Regards,
Manikanta.

>> +
>> +unevaluatedProperties: false
> 
> This changed... why?
> 
> 
> Best regards,
> Krzysztof
>
Manikanta Mylavarapu June 14, 2023, 11:43 a.m. UTC | #12
On 6/7/2023 1:57 PM, Krzysztof Kozlowski wrote:
> On 07/06/2023 10:10, Manikanta Mylavarapu wrote:
>>
>>
>> On 6/6/2023 7:19 PM, Kalle Valo wrote:
>>> Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes:
>>>
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      compatible:
>>>>>>>> +        enum:
>>>>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>>>>
>>>>>>> Keep rather alphabetical order (so both 5018 together).
>>>>>>>
>>>>>>> I also do not understand these at all. Why adding bus type to
>>>>>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>>>>>> soc).
>>>>>>>
>>>>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>>>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>>>>>> radio's properties, i have added bus type to compatible.
>>>>>
>>>>> It's the same device - WCSS - right? We do not create multiple nodes and
>>>>> compatibles for the same devices. Bus suffixes are almost never parts of
>>>>> compatibles.
>>>>
>>>>
>>>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
>>>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>>>>
>>>> So for better clarity i will use attached SOC ID in compatible.
>>>> Below are the new compatible's.
>>>>
>>>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
>>>> - qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
>>>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio
>>>
>>> What mandates that there's just one QCN6122 device attached to PCI?
>>> Assuming fixed PCI configurations like that makes me worried.
>>>
>>
>> IPQ5018 always has one internal radio, attached pcie radio's depends on
>> no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two
>> qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's
>> number of pcie devices controlled by QDSP6.
> 
> So this is hot-pluggable (or at least board-pluggable), then should not
> be a part of static DTS.
> 
> Some concepts of virtual-processes is anyway far away from hardware
> description, thus does not fit into DTS. Adding now to the equation PCIe
> with variable number of such processes, brings us even further.
> 
> This is not a DT property. Remember - DT describes hardware.
> 
> Best regards,
> Krzysztof
> 

In the multipd architecture based Socs, There is one Q6 DSP which runs 
the OS/kernel and there are one or more instances of WCSS radios
(It can be either internal or pcie attached).
These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios out 
of reset/ shuts it down etc). Q6 forms the 'root Protection domain' and 
the wcss radios are termed as the 'user protection domain'.
The compatible's that is being added here are to manage the 'root 
domain' and 'user domain'.
Not sure if using the words 'pcie'/'ahb' made it confusing.
So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'.

There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT based 
on number of wcss radios connected on that board and only one instance 
of 'qcom,ipq5018-q6-mpd'.

Is this approach ok ?

Thanks & Regards,
Manikanta.
Manikanta Mylavarapu June 14, 2023, 12:49 p.m. UTC | #13
On 6/14/2023 5:13 PM, Manikanta Mylavarapu wrote:
> 
> 
> On 6/7/2023 1:57 PM, Krzysztof Kozlowski wrote:
>> On 07/06/2023 10:10, Manikanta Mylavarapu wrote:
>>>
>>>
>>> On 6/6/2023 7:19 PM, Kalle Valo wrote:
>>>> Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes:
>>>>
>>>>>>>>> +
>>>>>>>>> +    properties:
>>>>>>>>> +      compatible:
>>>>>>>>> +        enum:
>>>>>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>>>>>
>>>>>>>> Keep rather alphabetical order (so both 5018 together).
>>>>>>>>
>>>>>>>> I also do not understand these at all. Why adding bus type to
>>>>>>>> compatible? This rarely is allowed (unless it is PCIe controller 
>>>>>>>> within
>>>>>>>> soc).
>>>>>>>>
>>>>>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>>>>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, 
>>>>>>> PCIE
>>>>>>> radio's properties, i have added bus type to compatible.
>>>>>>
>>>>>> It's the same device - WCSS - right? We do not create multiple 
>>>>>> nodes and
>>>>>> compatibles for the same devices. Bus suffixes are almost never 
>>>>>> parts of
>>>>>> compatibles.
>>>>>
>>>>>
>>>>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
>>>>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>>>>>
>>>>> So for better clarity i will use attached SOC ID in compatible.
>>>>> Below are the new compatible's.
>>>>>
>>>>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
>>>>> - qcom,ipq9574-wcss-mpd    //IPQ9574 internal radio
>>>>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio
>>>>
>>>> What mandates that there's just one QCN6122 device attached to PCI?
>>>> Assuming fixed PCI configurations like that makes me worried.
>>>>
>>>
>>> IPQ5018 always has one internal radio, attached pcie radio's depends on
>>> no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two
>>> qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's
>>> number of pcie devices controlled by QDSP6.
>>
>> So this is hot-pluggable (or at least board-pluggable), then should not
>> be a part of static DTS.
>>
>> Some concepts of virtual-processes is anyway far away from hardware
>> description, thus does not fit into DTS. Adding now to the equation PCIe
>> with variable number of such processes, brings us even further.
>>
>> This is not a DT property. Remember - DT describes hardware.
>>
>> Best regards,
>> Krzysztof
>>
> 
> In the multipd architecture based Socs, There is one Q6 DSP which runs 
> the OS/kernel and there are one or more instances of WCSS radios
> (It can be either internal or pcie attached).
> These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios out 
> of reset/ shuts it down etc). Q6 forms the 'root Protection domain' and 
> the wcss radios are termed as the 'user protection domain'.
> The compatible's that is being added here are to manage the 'root 
> domain' and 'user domain'.
> Not sure if using the words 'pcie'/'ahb' made it confusing.
> So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'.
> 
> There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT based 
> on number of wcss radios connected on that board and only one instance 
> of 'qcom,ipq5018-q6-mpd'.
> 
> Is this approach ok ?
> 
> Thanks & Regards,
> Manikanta.


I didn't aligned previous reply properly. Now i corrected it
and re sending.

In the multipd architecture based Socs, There is one Q6 DSP which
runs the OS/kernel and there are one or more instances of WCSS radios
(It can be either internal or pcie attached).
These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios
out of reset/ shuts it down etc).
Q6 forms the 'root Protection domain' and the wcss radios are termed
as the 'user protection domain'. The compatible's that is being
added here are to manage the 'root domain' and 'user domain'.
Not sure if using the words 'pcie'/'ahb' made it confusing.
So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'.

There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT
based on number of wcss radios connected on that board and only
one instance of 'qcom,ipq5018-q6-mpd'.

Is this approach ok ?

Thanks & Regards,
Manikanta.
Krzysztof Kozlowski June 14, 2023, 1:59 p.m. UTC | #14
On 14/06/2023 13:43, Manikanta Mylavarapu wrote:
>>>>>>>>> +    properties:
>>>>>>>>> +      compatible:
>>>>>>>>> +        enum:
>>>>>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>>>>>
>>>>>>>> Keep rather alphabetical order (so both 5018 together).
>>>>>>>>
>>>>>>>> I also do not understand these at all. Why adding bus type to
>>>>>>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>>>>>>> soc).
>>>>>>>>
>>>>>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>>>>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>>>>>>> radio's properties, i have added bus type to compatible.
>>>>>>
>>>>>> It's the same device - WCSS - right? We do not create multiple nodes and
>>>>>> compatibles for the same devices. Bus suffixes are almost never parts of
>>>>>> compatibles.
>>>>>
>>>>>
>>>>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
>>>>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>>>>>
>>>>> So for better clarity i will use attached SOC ID in compatible.
>>>>> Below are the new compatible's.
>>>>>
>>>>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
>>>>> - qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
>>>>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio
>>>>
>>>> What mandates that there's just one QCN6122 device attached to PCI?
>>>> Assuming fixed PCI configurations like that makes me worried.
>>>>
>>>
>>> IPQ5018 always has one internal radio, attached pcie radio's depends on
>>> no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two
>>> qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's
>>> number of pcie devices controlled by QDSP6.
>>
>> So this is hot-pluggable (or at least board-pluggable), then should not
>> be a part of static DTS.
>>
>> Some concepts of virtual-processes is anyway far away from hardware
>> description, thus does not fit into DTS. Adding now to the equation PCIe
>> with variable number of such processes, brings us even further.
>>
>> This is not a DT property. Remember - DT describes hardware.
>>
>> Best regards,
>> Krzysztof
>>
> 
> In the multipd architecture based Socs, There is one Q6 DSP which runs 
> the OS/kernel and there are one or more instances of WCSS radios
> (It can be either internal or pcie attached).
> These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios out 
> of reset/ shuts it down etc). Q6 forms the 'root Protection domain' and 
> the wcss radios are termed as the 'user protection domain'.
> The compatible's that is being added here are to manage the 'root 
> domain' and 'user domain'.
> Not sure if using the words 'pcie'/'ahb' made it confusing.
> So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'.
> 
> There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT based 
> on number of wcss radios connected on that board and only one instance 
> of 'qcom,ipq5018-q6-mpd'.
> 

I don't understand why the user protection domains need a specific
compatible. Why do they need compatible at all?

Not mentioning that amount of your domains on Q6 is actually fixed per
SoC and probably should not be in DT at all.

Qualcomm puts so many so weird stuff into DT which is not a hardware
description. I understand that everything is there a firmware, but then
make it discoverable for example...

Best regards,
Krzysztof
Manikanta Mylavarapu June 21, 2023, 11:39 a.m. UTC | #15
On 6/14/2023 7:29 PM, Krzysztof Kozlowski wrote:
> On 14/06/2023 13:43, Manikanta Mylavarapu wrote:
>>>>>>>>>> +    properties:
>>>>>>>>>> +      compatible:
>>>>>>>>>> +        enum:
>>>>>>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>>>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>>>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>>>>>>
>>>>>>>>> Keep rather alphabetical order (so both 5018 together).
>>>>>>>>>
>>>>>>>>> I also do not understand these at all. Why adding bus type to
>>>>>>>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>>>>>>>> soc).
>>>>>>>>>
>>>>>>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>>>>>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>>>>>>>> radio's properties, i have added bus type to compatible.
>>>>>>>
>>>>>>> It's the same device - WCSS - right? We do not create multiple nodes and
>>>>>>> compatibles for the same devices. Bus suffixes are almost never parts of
>>>>>>> compatibles.
>>>>>>
>>>>>>
>>>>>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
>>>>>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>>>>>>
>>>>>> So for better clarity i will use attached SOC ID in compatible.
>>>>>> Below are the new compatible's.
>>>>>>
>>>>>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
>>>>>> - qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
>>>>>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio
>>>>>
>>>>> What mandates that there's just one QCN6122 device attached to PCI?
>>>>> Assuming fixed PCI configurations like that makes me worried.
>>>>>
>>>>
>>>> IPQ5018 always has one internal radio, attached pcie radio's depends on
>>>> no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two
>>>> qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's
>>>> number of pcie devices controlled by QDSP6.
>>>
>>> So this is hot-pluggable (or at least board-pluggable), then should not
>>> be a part of static DTS.
>>>
>>> Some concepts of virtual-processes is anyway far away from hardware
>>> description, thus does not fit into DTS. Adding now to the equation PCIe
>>> with variable number of such processes, brings us even further.
>>>
>>> This is not a DT property. Remember - DT describes hardware.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> In the multipd architecture based Socs, There is one Q6 DSP which runs
>> the OS/kernel and there are one or more instances of WCSS radios
>> (It can be either internal or pcie attached).
>> These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios out
>> of reset/ shuts it down etc). Q6 forms the 'root Protection domain' and
>> the wcss radios are termed as the 'user protection domain'.
>> The compatible's that is being added here are to manage the 'root
>> domain' and 'user domain'.
>> Not sure if using the words 'pcie'/'ahb' made it confusing.
>> So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'.
>>
>> There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT based
>> on number of wcss radios connected on that board and only one instance
>> of 'qcom,ipq5018-q6-mpd'.
>>
> 
> I don't understand why the user protection domains need a specific
> compatible. Why do they need compatible at all?
> 
> Not mentioning that amount of your domains on Q6 is actually fixed per
> SoC and probably should not be in DT at all.
> 
   root domain is fixed per soc (One Q6 DSP, one per soc)
   user domain(s) are variable (based on number of wcss radios attached)

   The sequence to initialize, bring up, tear down the Q6 and wcss radios
   are completely different. So in order to differentiate them, we will
   need different compatibles. So this is a new rproc driver/architecture
   which has a parent/child topology (Q6 DSP -> Master/parent controls
   the WCSS (child)).

> Qualcomm puts so many so weird stuff into DT which is not a hardware
> description. I understand that everything is there a firmware, but then
> make it discoverable for example...
> 

    Hmm, in order to init/bring up these domains, clks, resets
    and some more register access are required. Q6 FW does not have
    access to all these resources. Also, HLOS rproc driver is needed to
    listen on all the events related to  these domains just like any
    other rproc driver.

Regards,
Manikanta
Krzysztof Kozlowski June 24, 2023, 7:19 a.m. UTC | #16
On 21/06/2023 13:39, Manikanta Mylavarapu wrote:
>>> on number of wcss radios connected on that board and only one instance
>>> of 'qcom,ipq5018-q6-mpd'.
>>>
>>
>> I don't understand why the user protection domains need a specific
>> compatible. Why do they need compatible at all?
>>
>> Not mentioning that amount of your domains on Q6 is actually fixed per
>> SoC and probably should not be in DT at all.
>>
>    root domain is fixed per soc (One Q6 DSP, one per soc)
>    user domain(s) are variable (based on number of wcss radios attached)
> 
>    The sequence to initialize, bring up, tear down the Q6 and wcss radios
>    are completely different. So in order to differentiate them, we will
>    need different compatibles. So this is a new rproc driver/architecture
>    which has a parent/child topology (Q6 DSP -> Master/parent controls
>    the WCSS (child)).

I understand you need different properties, but I don't see yet the
benefit of creating here artificial compatibles. Look at your ipq9574
DTSI change - it does not use even ipq9574 compatibles for 66% of its
children.

Maybe you should have there just property describing type of device or
bringup?

Given SoC cannot come with different amount of children (PD) and
different amount of radios. You even fix the firmware name, so
boards/customers cannot use anything else. It's fixed and the only
variable element here is disabling some of the blocks per board, if they
do not have physical interface (e.g. radio).

You even hard-code the number of PD by using "pd-[123]", without unit
address, so you do not expect it will grow.

Unless you want to say that these are devices? But your binding does not
really suggest it...


Best regards,
Krzysztof
Manikanta Mylavarapu June 30, 2023, 7:12 a.m. UTC | #17
On 6/24/2023 12:49 PM, Krzysztof Kozlowski wrote:
> On 21/06/2023 13:39, Manikanta Mylavarapu wrote:
>>>> on number of wcss radios connected on that board and only one instance
>>>> of 'qcom,ipq5018-q6-mpd'.
>>>>
>>>
>>> I don't understand why the user protection domains need a specific
>>> compatible. Why do they need compatible at all?
>>>
>>> Not mentioning that amount of your domains on Q6 is actually fixed per
>>> SoC and probably should not be in DT at all.
>>>
>>     root domain is fixed per soc (One Q6 DSP, one per soc)
>>     user domain(s) are variable (based on number of wcss radios attached)
>>
>>     The sequence to initialize, bring up, tear down the Q6 and wcss radios
>>     are completely different. So in order to differentiate them, we will
>>     need different compatibles. So this is a new rproc driver/architecture
>>     which has a parent/child topology (Q6 DSP -> Master/parent controls
>>     the WCSS (child)).
> 
> I understand you need different properties, but I don't see yet the
> benefit of creating here artificial compatibles. Look at your ipq9574
> DTSI change - it does not use even ipq9574 compatibles for 66% of its
> children.
> 
> Maybe you should have there just property describing type of device or
> bringup?
> 

	Yeah i got your point. Indeed the requirement here is to
	have device specific compatibles, so will have just two
	compatible one for Q6-MPD and another for WCSS-MPD device's


	"qcom,q6-mpd" --> For Q6-MPD device
	"qcom,wcss-mpd" --> For WCSS-MPD device

	Is this approach fine ?

> Given SoC cannot come with different amount of children (PD) and
> different amount of radios. You even fix the firmware name, so
> boards/customers cannot use anything else. It's fixed and the only
> variable element here is disabling some of the blocks per board, if they
> do not have physical interface (e.g. radio).
> 
> You even hard-code the number of PD by using "pd-[123]", without unit
> address, so you do not expect it will grow.
> 
> Unless you want to say that these are devices? But your binding does not
> really suggest it...
> 
> 
> 	Yes, as i mentioned above the requirement is to have device
	specific bindings. We will remove "PD-X" from soc dtsi and
	add it in board dts file.

	So soc dts would have "Q6-MPD" master node & board dts have
	"WCSS-MPD" child nodes based on number of radio's connected
	on board.

	Is this fine ?

Thanks & Regards,
Manikanta.
Krzysztof Kozlowski July 1, 2023, 10:51 a.m. UTC | #18
On 30/06/2023 09:12, Manikanta Mylavarapu wrote:
> 
> 
> On 6/24/2023 12:49 PM, Krzysztof Kozlowski wrote:
>> On 21/06/2023 13:39, Manikanta Mylavarapu wrote:
>>>>> on number of wcss radios connected on that board and only one instance
>>>>> of 'qcom,ipq5018-q6-mpd'.
>>>>>
>>>>
>>>> I don't understand why the user protection domains need a specific
>>>> compatible. Why do they need compatible at all?
>>>>
>>>> Not mentioning that amount of your domains on Q6 is actually fixed per
>>>> SoC and probably should not be in DT at all.
>>>>
>>>     root domain is fixed per soc (One Q6 DSP, one per soc)
>>>     user domain(s) are variable (based on number of wcss radios attached)
>>>
>>>     The sequence to initialize, bring up, tear down the Q6 and wcss radios
>>>     are completely different. So in order to differentiate them, we will
>>>     need different compatibles. So this is a new rproc driver/architecture
>>>     which has a parent/child topology (Q6 DSP -> Master/parent controls
>>>     the WCSS (child)).
>>
>> I understand you need different properties, but I don't see yet the
>> benefit of creating here artificial compatibles. Look at your ipq9574
>> DTSI change - it does not use even ipq9574 compatibles for 66% of its
>> children.
>>
>> Maybe you should have there just property describing type of device or
>> bringup?
>>
> 
> 	Yeah i got your point. Indeed the requirement here is to
> 	have device specific compatibles, so will have just two
> 	compatible one for Q6-MPD and another for WCSS-MPD device's
> 
> 
> 	"qcom,q6-mpd" --> For Q6-MPD device
> 	"qcom,wcss-mpd" --> For WCSS-MPD device
> 
> 	Is this approach fine ?

Can you fix your reply style, so it is like on every mailing list? Some
weird indentation does no help to read it.

I was proposing to drop compatibles entirely. Making compatibles generic
is not solving any of my concerns.

I don't understand what do you want to achieve here and very limited
description of the hardware in the binding does not help.

> 
>> Given SoC cannot come with different amount of children (PD) and
>> different amount of radios. You even fix the firmware name, so
>> boards/customers cannot use anything else. It's fixed and the only
>> variable element here is disabling some of the blocks per board, if they
>> do not have physical interface (e.g. radio).
>>
>> You even hard-code the number of PD by using "pd-[123]", without unit
>> address, so you do not expect it will grow.
>>
>> Unless you want to say that these are devices? But your binding does not
>> really suggest it...
>>
>>
>> 	Yes, as i mentioned above the requirement is to have device

What requirement? You did not describe anything. Binding describes
hardware, not your requirements.

Binding said nothing about devices.

> 	specific bindings. We will remove "PD-X" from soc dtsi and
> 	add it in board dts file.

Why? How is it related to the bindings? What does it solve? Instead of
doing some changes you should explain why.

> 
> 	So soc dts would have "Q6-MPD" master node & board dts have
> 	"WCSS-MPD" child nodes based on number of radio's connected
> 	on board.
> 
> 	Is this fine ?
> 

Why?

Best regards,
Krzysztof
Manikanta Mylavarapu July 18, 2023, 12:10 p.m. UTC | #19
On 7/1/2023 4:21 PM, Krzysztof Kozlowski wrote:
> On 30/06/2023 09:12, Manikanta Mylavarapu wrote:
>>
>>
>> On 6/24/2023 12:49 PM, Krzysztof Kozlowski wrote:
>>> On 21/06/2023 13:39, Manikanta Mylavarapu wrote:
>>>>>> on number of wcss radios connected on that board and only one instance
>>>>>> of 'qcom,ipq5018-q6-mpd'.
>>>>>>
>>>>>
>>>>> I don't understand why the user protection domains need a specific
>>>>> compatible. Why do they need compatible at all?
>>>>>
>>>>> Not mentioning that amount of your domains on Q6 is actually fixed per
>>>>> SoC and probably should not be in DT at all.
>>>>>
>>>>      root domain is fixed per soc (One Q6 DSP, one per soc)
>>>>      user domain(s) are variable (based on number of wcss radios attached)
>>>>
>>>>      The sequence to initialize, bring up, tear down the Q6 and wcss radios
>>>>      are completely different. So in order to differentiate them, we will
>>>>      need different compatibles. So this is a new rproc driver/architecture
>>>>      which has a parent/child topology (Q6 DSP -> Master/parent controls
>>>>      the WCSS (child)).
>>>
>>> I understand you need different properties, but I don't see yet the
>>> benefit of creating here artificial compatibles. Look at your ipq9574
>>> DTSI change - it does not use even ipq9574 compatibles for 66% of its
>>> children.
>>>
>>> Maybe you should have there just property describing type of device or
>>> bringup?
>>>
>>
>> 	Yeah i got your point. Indeed the requirement here is to
>> 	have device specific compatibles, so will have just two
>> 	compatible one for Q6-MPD and another for WCSS-MPD device's
>>
>>
>> 	"qcom,q6-mpd" --> For Q6-MPD device
>> 	"qcom,wcss-mpd" --> For WCSS-MPD device
>>
>> 	Is this approach fine ?
> 
> Can you fix your reply style, so it is like on every mailing list? Some
> weird indentation does no help to read it.
>

Sure, i will change my reply style and don't use any indentation.
Sorry for inconvenience.

> I was proposing to drop compatibles entirely. Making compatibles generic
> is not solving any of my concerns.
> 
> I don't understand what do you want to achieve here and very limited
> description of the hardware in the binding does not help.
> 

Sure, i remove user pd compatibles. In root pd probe itself, user pd
remoteproc's are taken care. Also I updated diagram in cover page, it
gives enough information.

>>
>>> Given SoC cannot come with different amount of children (PD) and
>>> different amount of radios. You even fix the firmware name, so
>>> boards/customers cannot use anything else. It's fixed and the only
>>> variable element here is disabling some of the blocks per board, if they
>>> do not have physical interface (e.g. radio).
>>>
>>> You even hard-code the number of PD by using "pd-[123]", without unit
>>> address, so you do not expect it will grow.
>>>
>>> Unless you want to say that these are devices? But your binding does not
>>> really suggest it...
>>>
>>>
>>> 	Yes, as i mentioned above the requirement is to have device
> 
> What requirement? You did not describe anything. Binding describes
> hardware, not your requirements.
> 
> Binding said nothing about devices.
> 

Yeah got your point. So we removed userpd compatibles from dt-bindings.


>> 	specific bindings. We will remove "PD-X" from soc dtsi and
>> 	add it in board dts file.
> 
> Why? How is it related to the bindings? What does it solve? Instead of
> doing some changes you should explain why.
> 
>>
>> 	So soc dts would have "Q6-MPD" master node & board dts have
>> 	"WCSS-MPD" child nodes based on number of radio's connected
>> 	on board.
>>
>> 	Is this fine ?
>>
> 
> Why?
> 

"PD-X" controls user pd WCSS bring up. WCSS blocks may vary based on
number of radio's connected on board but QDSP6 is always present.
So i will keep Q6 node in soc dtsi file and move 'PD-X' node to board
dts file.

Thanks & Regards,
Manikanta.

> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
new file mode 100644
index 000000000000..3257f27dc569
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
@@ -0,0 +1,265 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Multipd Secure Peripheral Image Loader
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+  - Mathieu Poirier <mathieu.poirier@linaro.org>
+
+description:
+  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd
+  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC. Pd means protection
+  domain. It's similar to process in Linux. Here QDSP6 processor runs each
+  wifi radio functionality on a separate process. One process can't access
+  other process resources, so this is termed as PD i.e protection domain.
+
+properties:
+  compatible:
+    enum:
+      - qcom,ipq5018-q6-mpd
+      - qcom,ipq9574-q6-mpd
+
+  reg:
+    maxItems: 1
+
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: Firmware name of the Hexagon core
+
+  interrupts-extended:
+    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
+
+  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
+
+  qcom,smem-state-names:
+    description:
+      Names of the states used by the AP to signal the remote processor
+    items:
+      - const: shutdown
+      - const: stop
+
+  memory-region:
+    items:
+      - description: Q6 pd 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.
+
+patternProperties:
+  "^pd-1|pd-2|pd-3":
+    type: object
+    description:
+      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
+      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
+      device node.
+
+    properties:
+      compatible:
+        enum:
+          - qcom,ipq5018-wcss-ahb-mpd
+          - qcom,ipq9574-wcss-ahb-mpd
+          - qcom,ipq5018-wcss-pcie-mpd
+
+      firmware-name:
+        $ref: /schemas/types.yaml#/definitions/string-array
+        items:
+          - description: Firmware name of the Hexagon core
+
+      interrupts-extended:
+        items:
+          - description: Fatal interrupt
+          - description: Ready interrupt
+          - description: Spawn acknowledge interrupt
+          - description: Stop acknowledge interrupt
+
+      interrupt-names:
+        items:
+          - const: fatal
+          - const: ready
+          - const: spawn-ack
+          - const: stop-ack
+
+      qcom,smem-states:
+        $ref: /schemas/types.yaml#/definitions/phandle-array
+        description: States used by the AP to signal the remote processor
+        items:
+          - description: Shutdown WCSS pd
+          - description: Stop WCSS pd
+          - description: Spawn WCSS pd
+
+      qcom,smem-state-names:
+        description:
+          Names of the states used by the AP to signal the remote processor
+        items:
+          - const: shutdown
+          - const: stop
+          - const: spawn
+
+    required:
+      - compatible
+      - firmware-name
+      - interrupts-extended
+      - interrupt-names
+      - qcom,smem-states
+      - qcom,smem-state-names
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - firmware-name
+  - reg
+  - interrupts-extended
+  - interrupt-names
+  - qcom,smem-states
+  - qcom,smem-state-names
+  - memory-region
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq5018-q6-mpd
+    then:
+      properties:
+        firmware-name:
+          items:
+            - const: IPQ5018/q6_fw.mdt
+            - const: IPQ5018/m3_fw.mdt
+            - const: qcn6122/m3_fw.mdt
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq9574-q6-mpd
+    then:
+      properties:
+        firmware-name:
+          items:
+            - const: IPQ9574/q6_fw.mdt
+            - const: IPQ9574/m3_fw.mdt
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    q6v5_wcss: remoteproc@cd00000 {
+      compatible = "qcom,ipq5018-q6-mpd";
+      reg = <0x0cd00000 0x4040>;
+      firmware-name = "IPQ5018/q6_fw.mdt",
+                      "IPQ5018/m3_fw.mdt",
+                      "qcn6122/m3_fw.mdt";
+      interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
+                            <&wcss_smp2p_in 0 0>,
+                            <&wcss_smp2p_in 1 0>,
+                            <&wcss_smp2p_in 2 0>,
+                            <&wcss_smp2p_in 3 0>;
+      interrupt-names = "wdog",
+                        "fatal",
+                        "ready",
+                        "handover",
+                        "stop-ack";
+
+      qcom,smem-states = <&wcss_smp2p_out 0>,
+                         <&wcss_smp2p_out 1>;
+      qcom,smem-state-names = "shutdown",
+                              "stop";
+
+      memory-region = <&q6_region>;
+
+      glink-edge {
+        interrupts = <GIC_SPI 179 IRQ_TYPE_EDGE_RISING>;
+        label = "rtr";
+        qcom,remote-pid = <1>;
+        mboxes = <&apcs_glb 8>;
+      };
+
+      pd-1 {
+        compatible = "qcom,ipq5018-wcss-ahb-mpd";
+        firmware-name = "IPQ5018/q6_fw.mdt";
+        interrupts-extended = <&wcss_smp2p_in 8 0>,
+                              <&wcss_smp2p_in 9 0>,
+                              <&wcss_smp2p_in 12 0>,
+                              <&wcss_smp2p_in 11 0>;
+        interrupt-names = "fatal",
+                          "ready",
+                          "spawn-ack",
+                          "stop-ack";
+        qcom,smem-states = <&wcss_smp2p_out 8>,
+                           <&wcss_smp2p_out 9>,
+                           <&wcss_smp2p_out 10>;
+        qcom,smem-state-names = "shutdown",
+                                "stop",
+                                "spawn";
+      };
+
+      pd-2 {
+        compatible = "qcom,ipq5018-wcss-pcie-mpd";
+        firmware-name = "IPQ5018/q6_fw.mdt";
+        interrupts-extended = <&wcss_smp2p_in 16 0>,
+                              <&wcss_smp2p_in 17 0>,
+                              <&wcss_smp2p_in 20 0>,
+                              <&wcss_smp2p_in 19 0>;
+        interrupt-names = "fatal",
+                          "ready",
+                          "spawn-ack",
+                          "stop-ack";
+
+        qcom,smem-states = <&wcss_smp2p_out 16>,
+                           <&wcss_smp2p_out 17>,
+                           <&wcss_smp2p_out 18>;
+        qcom,smem-state-names = "shutdown",
+                                "stop",
+                                "spawn";
+      };
+
+      pd-3 {
+        compatible = "qcom,ipq5018-wcss-pcie-mpd";
+        firmware-name = "IPQ5018/q6_fw.mdt";
+        interrupts-extended = <&wcss_smp2p_in 24 0>,
+                              <&wcss_smp2p_in 25 0>,
+                              <&wcss_smp2p_in 28 0>,
+                              <&wcss_smp2p_in 27 0>;
+        interrupt-names = "fatal",
+                          "ready",
+                          "spawn-ack",
+                          "stop-ack";
+
+        qcom,smem-states = <&wcss_smp2p_out 24>,
+                           <&wcss_smp2p_out 25>,
+                           <&wcss_smp2p_out 26>;
+        qcom,smem-state-names = "shutdown",
+                                "stop",
+                                "spawn";
+       };
+    };