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