Re: [PATCH v3 1/2] dt-bindings: firmware: svc: Add IOMMU support for Agilex5

[Krzysztof Kozlowski <krzk@kernel.org>]

On 04/11/2025 12:14, Romli, Khairul Anuar wrote:
> On 4/11/2025 5:48 pm, Krzysztof Kozlowski wrote:
>> On Tue, Nov 04, 2025 at 07:39:27AM +0800, Khairul Anuar Romli wrote:
>>> In Agilex5, the TBU (Translation Buffer Unit) can now operate in non-secure
>>> mode, enabling Linux to utilize it through the IOMMU framework. This allows
>>> improved memory management capabilities in non-secure environments. With
>>> Agilex5 lifting this restriction, we are now extending the device tree
>>> bindings to support IOMMU for the Agilex5 SVC.
>>>
>>> Signed-off-by: Khairul Anuar Romli <khairul.anuar.romli@altera.com>
>>> Reviewed-by: Adrian Ng Ho Yin <adrianhoyin.ng@altera.com>
>>> Reviewed-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>
>>
>> Two reviews but...
>>
>>>   
>>> -required:
>>> -  - compatible
>>> -  - method
>>> -  - memory-region
>>> +allOf:
>>> +  - required:
>>> +      - compatible
>>> +      - method
>>> +      - memory-region
>>
>> ... none told you this is not the correct syntax / style?
>>
>> Were these reviews really happening? What exactly was reviewed here?
>>
>> Best regards,
>> Krzysztof
>>
> 
> The peer review was on the property and compatible added in this patch.
> I was at fault for not emphasize to check on the syntax / style for the 
> logical changes.

Rather the reviewers gave you just simple acks without doing their job.
That's a 10 liner patch...

Feels like just to tick gerrit or satisfy internal procedures.

If internal reviewers give you such "okay" marks, without adhering to
reviewers statement of oversight, you can freely ignore them and instead
educate them to provide full review on the mailing list.

> 
> I refers to 
> Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml 
> and the changes could be as follows:
> 
> required:
>    - compatible
>    - method
>    - memory-region
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: intel,agilex5-svc
> +    then:
> +      required:
> +        - iommus
> 
> Do I need to re-specify the rest of the compatible property other than 
> intel,agilex5-svc and their required properties under the allOf block?
> 
> My objective is to specify that iommus only for intel,agilex5-svc but 
> not the rest of devices.
It all depends whether other devices also use IOMMU.

Best regards,
Krzysztof