Re: [PATCH v4 5/5] arm64: versal-net: Switch Versal NET to firmware clock interface

[Michal Simek <michal.simek@amd.com>]

On 7/8/26 09:42, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Device tree binding headers are incorrectly placed in arch/arm64/boot/dts/xilinx/ and duplicate existing bindings.
> - [Medium] Use of #undef directives in device tree binding headers to remove inherited definitions.
> --
> 
> --- Patch [5]: [PATCH v4 5/5] arm64: versal-net: Switch Versal NET to firmware clock interface ---
> commit 768ce0b0d6c0eff83d929d6530cf30c0e1e4baaa
> Author: Michal Simek <michal.simek@amd.com>
> 
> arm64: versal-net: Switch Versal NET to firmware clock interface
> 
> Switch Versal NET from using fixed clocks to the firmware-based clock
> interface. Add DT macro headers for Versal NET and base Versal clocks,
> power domains, and mandatory resets required by the clock dtsi.
> 
>> diff --git a/arch/arm64/boot/dts/xilinx/xlnx-versal-clk.h b/arch/arm64/boot/dts/xilinx/xlnx-versal-clk.h
>> new file mode 100644
>> index 0000000000000..d0c4abf78f307
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/xilinx/xlnx-versal-clk.h
>> @@ -0,0 +1,123 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2019 - 2022, Xilinx, Inc.
>> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#ifndef _XLNX_VERSAL_CLK_H
>> +#define _XLNX_VERSAL_CLK_H
>> +
>> +#define PMC_PLL					1
> 
> [Severity: Medium]
> Should these device tree binding headers be placed in include/dt-bindings/
> instead of arch/arm64/boot/dts/xilinx/?
> 
> Placing them in the architecture directory bypasses the convention of having
> binding definitions act as a shared contract between the device tree and
> kernel drivers. This applies to xlnx-versal-clk.h, xlnx-versal-power.h,
> and others introduced here.

These are just holding constants and current file sin dt-bindings should be 
deprecated as is done by


commit f0ceedd52a69a8bf63778b1fe9e1c10e02ecd3fd
     dt-bindings: xilinx: Deprecate header with firmware constants


> 
> [ ... ]
> 
>> diff --git a/arch/arm64/boot/dts/xilinx/xlnx-versal-net-clk.h b/arch/arm64/boot/dts/xilinx/xlnx-versal-net-clk.h
>> new file mode 100644
>> index 0000000000000..4a6add03c173e
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/xilinx/xlnx-versal-net-clk.h
> [ ... ]
>> +/* Remove Versal specific node IDs */
>> +#undef APU_PLL
>> +#undef RPU_PLL
>> +#undef CPM_PLL
> 
> [Severity: Medium]
> Will these undef directives work correctly with the device tree schema tools?
> 
> Since schema validation tools typically use simple regex-based parsers rather
> than a full C preprocessor, they generally ignore undef statements. Could this
> cause the validation schemas to incorrectly accept the removed IDs?

validation is not affected.

> 
> [ ... ]
> 
>> diff --git a/arch/arm64/boot/dts/xilinx/xlnx-versal-resets.h b/arch/arm64/boot/dts/xilinx/xlnx-versal-resets.h
>> new file mode 100644
>> index 0000000000000..fe00f4a0ba65b
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/xilinx/xlnx-versal-resets.h
>> @@ -0,0 +1,106 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2020 - 2022, Xilinx, Inc.
>> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#ifndef _XLNX_VERSAL_RESETS_H
>> +#define _XLNX_VERSAL_RESETS_H
>> +
>> +#define VERSAL_RST_PMC_POR			(0xc30c001U)
> 
> [Severity: Medium]
> Does this new file unnecessarily duplicate the already existing reset IDs
> binding in include/dt-bindings/reset/xlnx-versal-resets.h?

as above.

Thanks,
Michal