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

[Tanmay Shah <tanmay.shah@amd.com>]

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
>