[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1027ec80-bed6-4b51-b346-60bc19aa04e6@amd.com>
Date: Wed, 13 Dec 2023 10:35:57 -0600
From: Tanmay Shah <tanmay.shah@....com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
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,
"Levinsky, Ben" <ben.levinsky@....com>
Subject: Re: [PATCH v2] dt-bindings: mailbox: add Versal IPI bindings
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
>
Powered by blists - more mailing lists