[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d91a9278-f70e-4f0f-92c1-ce0bdac69ff5@linaro.org>
Date: Wed, 13 Dec 2023 07:35:04 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Tanmay Shah <tanmay.shah@....com>, jassisinghbrar@...il.com,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, michal.simek@....com,
shubhrajyoti.datta@....com
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2] dt-bindings: mailbox: add Versal IPI bindings
On 13/12/2023 00:03, Tanmay Shah wrote:
> Add documentation for AMD-Xilinx Versal platform Inter Processor Interrupt
> controller. Versal IPI controller contains buffer-less IPI which do not
> have buffers for message passing. For such IPI channels message buffers
> are not expected and only notification to/from remote agent is expected.
>
> Signed-off-by: Tanmay Shah <tanmay.shah@....com>
> ---
>
> properties:
> compatible:
> - const: xlnx,zynqmp-ipi-mailbox
> + enum:
> + - xlnx,zynqmp-ipi-mailbox
> + - xlnx,versal-ipi-mailbox
>
> method:
> description: |
> The method of calling the PM-API firmware layer.
> - Permitted values are.
> - - "smc" : SMC #0, following the SMCCC
> - - "hvc" : HVC #0, following the SMCCC
> -
Independent change. Please do not mix logical changes in one patch.
> $ref: /schemas/types.yaml#/definitions/string
> enum:
> - smc
> @@ -58,16 +56,26 @@ properties:
> '#size-cells':
> const: 2
>
> - xlnx,ipi-id:
> - description: |
> - Remote Xilinx IPI agent ID of which the mailbox is connected to.
> - $ref: /schemas/types.yaml#/definitions/uint32
> + reg:
> + minItems: 1
> + maxItems: 2
> +
> + reg-names:
> + minItems: 1
> + maxItems: 2
I don't understand why this change is here. Previously you did not have
MMIO address space? If yes, then where do you restrict the old device to
disallow these?
>
> interrupts:
> maxItems: 1
>
> ranges: true
>
> + xlnx,ipi-id:
> + description: |
> + Remote Xilinx IPI agent ID of which the mailbox is connected to.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 64
> +
> patternProperties:
> '^mailbox@[0-9a-f]+$':
> description: Internal ipi mailbox node
> @@ -76,57 +84,116 @@ patternProperties:
> properties:
>
> compatible:
> - const: xlnx,zynqmp-ipi-dest-mailbox
> + enum:
> + - xlnx,zynqmp-ipi-dest-mailbox
> + - xlnx,versal-ipi-dest-mailbox
>
> - xlnx,ipi-id:
> - description:
> - Remote Xilinx IPI agent ID of which the mailbox is connected to.
> - $ref: /schemas/types.yaml#/definitions/uint32
> + reg:
> + minItems: 1
> + maxItems: 4
> +
> + reg-names:
> + minItems: 1
> + maxItems: 4
Same concern.
>
> '#mbox-cells':
> const: 1
> description:
> It contains tx(0) or rx(1) channel IPI id number.
>
> - reg:
> - maxItems: 4
> -
> - reg-names:
> - items:
> - - const: local_request_region
> - - const: local_response_region
> - - const: remote_request_region
> - - const: remote_response_region
> + xlnx,ipi-id:
> + description:
> + Remote Xilinx IPI agent ID of which the mailbox is connected to.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 64
>
> required:
> - compatible
> - reg
> - reg-names
> - "#mbox-cells"
> -
> -additionalProperties: false
> -
> + - xlnx,ipi-id
> +
> + allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - xlnx,zynqmp-ipi-dest-mailbox
> + then:
> + properties:
> + reg:
> + items:
> + - description: Host agent request message buffer
> + - description: Host agent response message buffer
> + - description: Remote agent request message buffer
> + - description: Remote agent response message buffer
> +
> + reg-names:
> + items:
> + - const: local_request_region
> + - const: local_response_region
> + - const: remote_request_region
> + - const: remote_response_region
> + else:
> + properties:
> + reg:
> + minItems: 1
> + items:
> + - description: Remote IPI agent control register
> + - description: Remote IPI agent optional message buffer
Were these described in old binding? If not, it's a separate change.
> +
> + reg-names:
> + minItems: 1
> + items:
> + - const: ctrl
> + - const: msg
Blank line
> required:
> - compatible
> - - interrupts
> - '#address-cells'
> - '#size-cells'
> + - interrupts
Separate change with its own rationale. Trivial cleanups can be
organized in one patch, but should not be mixed with adding new devices.
> - xlnx,ipi-id
>
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - xlnx,versal-ipi-mailbox
> + then:
> + properties:
> + reg:
> + items:
> + - description: Host IPI agent control registers
> + - description: Host IPI agent optional message buffers
> +
> + reg-names:
> + items:
> + - const: ctrl
> + - const: msg
> +
> + required:
> + - reg
> + - reg-names
> +
> +additionalProperties: false
> +
> +
Just one blank line.
> examples:
> - |
> #include<dt-bindings/interrupt-controller/arm-gic.h>
>
> - amba {
> - #address-cells = <0x2>;
> - #size-cells = <0x2>;
> + bus {
> zynqmp-mailbox {
> compatible = "xlnx,zynqmp-ipi-mailbox";
> interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
> xlnx,ipi-id = <0>;
> #address-cells = <2>;
> #size-cells = <2>;
> - ranges;
How is this related to Versal?
>
> mailbox: mailbox@...905c0 {
> compatible = "xlnx,zynqmp-ipi-dest-mailbox";
> @@ -144,4 +211,41 @@ examples:
> };
> };
>
> + - |
> + #include<dt-bindings/interrupt-controller/arm-gic.h>
> +
> + bus {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + zynqmp-mailbox@...00000 {
mailbox@
> + compatible = "xlnx,versal-ipi-mailbox";
> + interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> + reg = <0x0 0xff300000 0x0 0x1000>,
> + <0x0 0xff990000 0x0 0x1ff>;
> + reg-names = "ctrl", "msg";
> + xlnx,ipi-id = <0>;
> + ranges;
> +
Best regards,
Krzysztof
Powered by blists - more mailing lists