[v2] dt-bindings: mailbox: add Versal IPI bindings

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@amd.com>
---

Changes in v2:
  - Add versal bindings to existing bindings doc instead of separate
    file.
  - Sort required list same as properties list
  - Add minimum and maximum range for xlnx,ipi-id vendor property
  - Move vendor property last in the list
  - Fix description of child node reg property for versal bindings
  - Change commit text

depends on: https://lore.kernel.org/linux-arm-kernel/79f65b96-9015-41c4-b4ee-a82526c9eefc@linaro.org/T/#meeacc5c57a9610b19758d313e5b2d17ab470f646

 .../mailbox/xlnx,zynqmp-ipi-mailbox.yaml      | 172 ++++++++++++++----
 1 file changed, 138 insertions(+), 34 deletions(-)


base-commit: abb240f7a2bd14567ab53e602db562bb683391e6
prerequisite-patch-id: 70017c8eaded5fc85749995b9cf093c6c625fab3

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@amd.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@ff9905c0 {
>            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@ff300000 {

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

Hi Krzysztof,

Thanks for reviews. Please find my comments below inline.


On 12/13/23 12:35 AM, Krzysztof Kozlowski wrote:
> 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@amd.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?


Hardware description is different between ZynqMP and Versal. And so, we have to design

new bindings for Versal. reg and reg-names for parent node, is only available for new devices.

The new device is checked with compatible string after "required" section.

Should I add "unevaluatedProperties: false" after "additionalProperties: false" for parent node below [1] ?


>
> >  
> >    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.

This one, reg and reg-names are available for both old and new devices but, with different

definition. And this is checked based on compatible of child node below [2].


>
> >  
> >        '#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

[2] Here based on compatible string, "reg" and "reg-names" are defined.


> > +        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.

Okay, so I will split this in two patches:

1) Clean up current bindings (like remove redundant descriptino, sort "required" property order etc..)

2) Add versal platforms bindings doc. (This will add if else cases and Versal platform support)


> > +
> > +            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.

Ack


> >    - 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

[1] Here, if I add unevaluatedProperties: false then is it enough for old device to disallow

"reg" and "reg-names" for parent node ?

If not, could you please suggest how to achieve this?

Or is there a way to undefine "reg" and "reg-names" based on compatible property ?

Based on your feedback, I will post v3.


> > +
> > +
>
> Just one blank line.

Ack.


>
> >  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?

Yeah its not, will go in cleanup patch.

>
> >  
> >          mailbox: mailbox@ff9905c0 {
> >            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@ff300000 {
>
> mailbox@

Ack.

>
> > +        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
>

On 13/12/2023 17:18, Tanmay Shah wrote:


>>> +    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?
> 
> 
> Hardware description is different between ZynqMP and Versal. And so, we have to design
> 
> new bindings for Versal. reg and reg-names for parent node, is only available for new devices.
> 
> The new device is checked with compatible string after "required" section.

Hm? What does that mean?

> 
> Should I add "unevaluatedProperties: false" after "additionalProperties: false" for parent node below [1] ?

No, you should disallow reg/reg-names for older device.

...

> 
>>> +        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.
> 
> Okay, so I will split this in two patches:
> 
> 1) Clean up current bindings (like remove redundant descriptino, sort "required" property order etc..)

If you want to combine cleanup and functional changes, then this would
be two patches.

> 
> 2) Add versal platforms bindings doc. (This will add if else cases and Versal platform support)

> 
> 
>>> +
>>> +            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.
> 
> Ack
> 
> 
>>>    - 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
> 
> [1] Here, if I add unevaluatedProperties: false then is it enough for old device to disallow

No, you need reg:false

Best regards,
Krzysztof

On 12/13/23 10:21 AM, Krzysztof Kozlowski wrote:
> On 13/12/2023 17:18, Tanmay Shah wrote:
>
>
> >>> +    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?
> > 
> > 
> > Hardware description is different between ZynqMP and Versal. And so, we have to design
> > 
> > new bindings for Versal. reg and reg-names for parent node, is only available for new devices.
> > 
> > The new device is checked with compatible string after "required" section.
>
> Hm? What does that mean?


Following [1]. I think You answered what needs to be done. I will have reg: false for old devices.

Thanks.


>
> > 
> > Should I add "unevaluatedProperties: false" after "additionalProperties: false" for parent node below [1] ?
>
> No, you should disallow reg/reg-names for older device.
>
> ...
>
> > 
> >>> +        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.
> > 
> > Okay, so I will split this in two patches:
> > 
> > 1) Clean up current bindings (like remove redundant descriptino, sort "required" property order etc..)
>
> If you want to combine cleanup and functional changes, then this would
> be two patches.
>
> > 
> > 2) Add versal platforms bindings doc. (This will add if else cases and Versal platform support)

In that case, can I just post single patch that adds new device support ?

Won't touch anything that is not needed for new device support.


>
> > 
> > 
> >>> +
> >>> +            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.
> > 
> > Ack
> > 
> > 
> >>>    - 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
> > 
> > [1] Here, if I add unevaluatedProperties: false then is it enough for old device to disallow
>
> No, you need reg:false
>
> Best regards,
> Krzysztof
>