Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <1027ec80-bed6-4b51-b346-60bc19aa04e6@amd.com>
Date:   Wed, 13 Dec 2023 10:35:57 -0600
User-Agent: Mozilla Thunderbird Beta
Subject: Re: [PATCH v2] dt-bindings: mailbox: add Versal IPI bindings
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        jassisinghbrar@gmail.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        michal.simek@amd.com, shubhrajyoti.datta@amd.com
Cc:     linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        "Levinsky, Ben" <ben.levinsky@amd.com>
References: <20231212230353.888714-1-tanmay.shah@amd.com>
 <d91a9278-f70e-4f0f-92c1-ce0bdac69ff5@linaro.org>
 <44052047-06c9-4b95-b040-2d4c7c1343d1@amd.com>
 <daa14201-2027-4732-bbea-acbb2675e4b8@linaro.org>
From:   Tanmay Shah <tanmay.shah@amd.com>
In-Reply-To: <daa14201-2027-4732-bbea-acbb2675e4b8@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NEhnVmdQMUFkdlFWNEFFaFMrb05NbVZsQ0NDaXdMMlBtek9RS25xemVLQ1dw?=
 =?utf-8?B?SkM5L0lyRHp1TzI4eWhjY2l4Qm56TndRc3dHQ0EreCtBZjVraFlGSGQ2dlFU?=
 =?utf-8?B?TEFPOUs2V1NYQ2Z3c0ZndElRTHhGdGM5WVJsSWZOVS9Jd08vK1JiVGtsa2to?=
 =?utf-8?B?NnNtQXl3N09Qa3YrTEM0VGUzU2VObUFTYlRYRkRuVnl2NUdaZFhYYk1qNDA0?=
 =?utf-8?B?UnRyNU5KS0dzZ3BpSHAyZUtvb0xMd1FvWVp6WGMwRE5WZVFBc0RqRXU1bUhF?=
 =?utf-8?B?T042Qld4aEZFK1JTTEh4N0VQblRUOTh2VzMrZTdGck5od0hhUUthRU9MZmsz?=
 =?utf-8?B?TW1zUFJSbTZxQzBacTJ0STZsQ2pvV28xZzRIQytlMTU2cnQxNzRvK0J3UE1n?=
 =?utf-8?B?TDAwVUlKV0xkRXRoNXhsT1g5RTZJNWN0YXNCbmRYbWpaRDl0SUdGWm9QUmNa?=
 =?utf-8?B?M28wa1dORENHK21GMmpmZzNBRENrcDlnZDA2OGxUMnpiNzJ0RlduUi84U2x4?=
 =?utf-8?B?V28wVGN3UHdIZDhIK2NCYkorb0R0V1dHa29hM1pRVFY0WlNiaUR0YU0zVlNY?=
 =?utf-8?B?Kzg5c1ltdWVYR2tsaEZZRWdIeWlUcmUrTzY1VTZKWHQzVFZXSFl6V1dwMlpT?=
 =?utf-8?B?YVNEK3YwakZuMjdBMXJhU2N4aWtnaHJxOVBWYmRSWGEvUHNYZHRvT3Rzdmd2?=
 =?utf-8?B?WEd2V3NSVGdHRnp1ZmsyeDdYNklPSnRGNkllMUZHeGE4encvSWxYeHZrM2Mr?=
 =?utf-8?B?WEt5bVI4RzNucjYyWmhzb1ZLa1pxYmNTU2ZrMXBremRDbWp4VFpjbUZucEJn?=
 =?utf-8?B?b2FWN0srOG1GVHkyanc1dDVKbGY0eU1FWmd6enMwWC85djBqVnhjdThoOVpO?=
 =?utf-8?B?RCtDSk96Q3dkVEcvV3ZiNncxQ0E2Sm1taE5kdnBldHVVWHVzc0Z0Tk5jSVQ5?=
 =?utf-8?B?K0FhM3p2WlhPSDZVR3JxVWVSRjEwZ0hmZmdQLzZ3anMvc3FER2Ntd0R1SXla?=
 =?utf-8?B?akVKUFI3QVU4Q1Iwb0pVckU3TzVaNzYya2MvWVNES3BKVGpGaS9zZzYvcW10?=
 =?utf-8?B?YzlTWmlsUGF2eXpqLysrdWdhcFpJQ3EvNDA0WUtjVWtQdGkzSkREN1lSU1hW?=
 =?utf-8?B?aURBSHlxZTRLbUZ1UkRiSUM3TU9CYzlQSDZud0dNWXdYR3ljendSQzl1ZktK?=
 =?utf-8?B?OGM3QkxrR2JoTVNqK2I2QWtyNTlaRW5sNnV5UzIzMVRxTUlQTXJ4dzdWWnlR?=
 =?utf-8?B?K0IrN1JzWjVrRnFNdUlMNG9sdGV4TytsVmhNQWFSaU1IRXBPL0VXYWhxdEtN?=
 =?utf-8?B?WU5nU2xZTTBoT0R3VXpoSDEvOXZpemkrY21LaXJsU3dWZ1NVaXk5UkJ1WEdv?=
 =?utf-8?B?Zy8zVnFsK2FBWFJoc2U5ZjBJY1BNUkJwRmp5YzdsYTVFclNjNllGdnFzZ0lN?=
 =?utf-8?B?Y3N1aE15VmQ4MFN0aC9XUzdaVElENHlaVk1kZml1d3NCSXlhZDltZHA0MkRz?=
 =?utf-8?B?N1FZaG1xSzhTbkJWTzlOYnVJOWNpd1Y5THNtRlExRHUvL2d2bTV1dzJMbWNp?=
 =?utf-8?B?ZDdmUFlyVUtjSTFidjJHekNuQWhucHEwWmdpU0hZUkQzWHdsNXA0WU55RTB3?=
 =?utf-8?B?eEpyNXBNbUxhZ2R2N04xb1dxMS93STl6WnhxSm1RM3ZKQVpKN1oxeW02UHl2?=
 =?utf-8?B?SUIyanpGeWV1aWsvYVJ6UElhTUtMREtlOWgyempsUkFoV2Fxb05GdUlFR3dS?=
 =?utf-8?B?SzNJcW1aUllPYkJQSGYvNGpKTjJmdnQvL1NBNDZsUVQ4bWRSRkNBNW50NFZI?=
 =?utf-8?B?cGxqUHJVUnpmYUhHREFiQ1lnZjRWdFRJVDNqOHc2N0NsS2dDS0tWQU44Z1ha?=
 =?utf-8?B?UFVyM1RxV1FyY1h3cWZnSURmUE9yaStsLzZnVlBJbmNHeHJtb2pCTWtPa1Zr?=
 =?utf-8?B?Y3NidjBQUjZQaXVWVUpCYWlsUGlKcUFhRzV2U0o5Y0Nja2Y1RmVGVHpLRXl5?=
 =?utf-8?B?VFNTeEJiWmRWWmVodlZyZ1lmTERidWt5NU43NHhRS0pwZlhVeERYMnB1bkR6?=
 =?utf-8?B?dzZGbjZucFJxWGU4b3VZbW84S1pza0VrYUZzVjR6ZThOaTBnRGVib200QjUw?=
 =?utf-8?Q?kwosMQtbNC/7v6XgsXDV6VFBK?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b3cccd6-3e01-41ac-632d-08dbfbf99739
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB3683.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 16:36:00.7045
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vCSC/2K27+fY74p/OCuvBy8JseOD675zYiuXIM5hXQqDpcpHAmq+a32IOvpSD1Af
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB5949
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Wed, 13 Dec 2023 08:36:30 -0800 (PST)


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
>