Re: [PATCH 2/5] dt-bindings: clock: altr,agilex72-clkmgr: add Agilex72 clock manager binding

["Chiam, Evon" <evon.chiam@altera.com>]

Hi Krzysztof,

Please find my replies inline below.

On 18/8/2026 4:23 pm, Krzysztof Kozlowski wrote:
> On 06/08/2026 11:25, evon.chiam@altera.com wrote:
>> From: "Chiam, Evon" <evon.chiam@altera.com>
>>
>> Add binding and clock ID header for Altera SoCFPGA Agilex72 clock manager.
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> 
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23
Will address these in v2.>
>>
>> Signed-off-by: Chiam, Evon <evon.chiam@altera.com>
> 
> 
> 
>> +
>> +examples:
>> +  - |
>> +    /* External reference clocks supplied to the clock manager */
>> +    clocks {
>> +      cb_intosc_div2_clk: cb-intosc-div2-clk {
>> +        #clock-cells = <0>;
>> +        compatible = "fixed-clock";
>> +        clock-frequency = <0>;
>> +      };
>> +
>> +      /* cb-intosc divided by 2, then by 5 (total divide-by-10) */
>> +      cb_intosc_div10_clk: cb-intosc-div10-clk {
>> +        #clock-cells = <0>;
>> +        compatible = "fixed-factor-clock";
>> +        clocks = <&cb_intosc_div2_clk>;
>> +        clock-mult = <1>;
>> +        clock-div = <5>;
>> +      };
>> +
>> +      f2s_free_clk: f2s-free-clk {
>> +        #clock-cells = <0>;
>> +        compatible = "fixed-clock";
>> +        clock-frequency = <0>;
>> +      };
>> +
>> +      osc1: osc1 {
>> +        #clock-cells = <0>;
>> +        compatible = "fixed-clock";
>> +        clock-frequency = <0>;
>> +      };
>> +    };
> 
> None of above are relevant. Drop.
> 
Will drop them in v2.>> +
>> +    clkmgr: clock-controller@915c000 {
>> +      compatible = "altr,agilex72-clkmgr";
>> +      reg = <0x915c000 0x1000>,
>> +            <0x915e000 0x1000>,
>> +            <0x915f000 0x1000>,
>> +            <0x9160000 0x1000>;
>> +      reg-names = "clkmgr", "gppll0", "gppll1", "gppll2";
>> +      #clock-cells = <1>;
>> +    };
> 
> 
> ...
> 
>> +#define AGILEX72_COMP0_CLK             36
>> +#define AGILEX72_CORE2_CLK             37
>> +#define AGILEX72_CORE3_CLK             38
>> +#define AGILEX72_MPU_CLK               39
>> +#define AGILEX72_CCU_CLK               40
>> +#define AGILEX72_APU_SYS_FREE_CLK      41
>> +#define AGILEX72_HSP_SYS_FREE_CLK      42
>> +#define AGILEX72_HSP_MAIN_FREE_CLK     43
>> +#define AGILEX72_HSP_MAIN_CLK          44
>> +#define AGILEX72_HSP_MP_CLK            45
>> +#define AGILEX72_HSP_SP_CLK            46
>> +#define AGILEX72_USB2OTG_HCLK          47
>> +#define AGILEX72_LSP_SYS_FREE_CLK      48
>> +#define AGILEX72_LSP_MAIN_FREE_CLK     49
>> +#define AGILEX72_LSP_MAIN_CLK          50
>> +#define AGILEX72_LSP_MP_CLK            51
>> +#define AGILEX72_LSP_SP_CLK            52
>> +#define AGILEX72_SPIM_0_CLK            53
>> +#define AGILEX72_SPIM_1_CLK            54
>> +#define AGILEX72_SPIS_0_CLK            55
>> +#define AGILEX72_SPIS_1_CLK            56
>> +#define AGILEX72_DMA_0_CORE_CLK        57
>> +#define AGILEX72_DMA_0_HS_CLK          58
>> +#define AGILEX72_DMA_1_CORE_CLK        59
>> +#define AGILEX72_DMA_1_HS_CLK          60
>> +#define AGILEX72_I3C_0_CORE_CLK        61
>> +#define AGILEX72_I3C_1_CORE_CLK        62
>> +#define AGILEX72_I2C_0_PCLK            63
>> +#define AGILEX72_I2C_1_PCLK            64
>> +#define AGILEX72_I2C_EMAC0_PCLK        65
>> +#define AGILEX72_I2C_EMAC1_PCLK        66
>> +#define AGILEX72_I2C_EMAC2_PCLK        67
>> +#define AGILEX72_UART_0_PCLK           68
>> +#define AGILEX72_UART_1_PCLK           69
>> +#define AGILEX72_UART_2_PCLK           70
>> +#define AGILEX72_SPTIMER_0_PCLK        71
>> +#define AGILEX72_SPTIMER_1_PCLK        72
>> +#define AGILEX72_CS_AT_CLK             73
>> +#define AGILEX72_CS_PDBG_CLK           74
>> +#define AGILEX72_CS_TRACE_CLK          75
>> +#define AGILEX72_EMACA_DIV_CLK         76
>> +#define AGILEX72_EMACB_DIV_CLK         77
>> +#define AGILEX72_EMAC0_CLK             78
>> +#define AGILEX72_EMAC1_CLK             79
>> +#define AGILEX72_EMAC2_CLK             80
>> +#define AGILEX72_EMAC_PTP_CLK          81
>> +#define AGILEX72_GPIO_DB_CLK           82
>> +#define AGILEX72_USB31_SUSPEND_CLK     83
>> +#define AGILEX72_USB31_BUS_CLK_EARLY   84
>> +#define AGILEX72_S2F_USER0_CLK         85
>> +#define AGILEX72_S2F_USER1_CLK         86
>> +#define AGILEX72_XSPI_PCLK             87
>> +#define AGILEX72_XSPI_CLK              88
>> +#define AGILEX72_XSPI_PHY_CLK          89
>> +#define AGILEX72_SDMMC0_SDPHY_REG_CLK  90
>> +#define AGILEX72_SDMMC1_SDPHY_REG_CLK  91
>> +#define AGILEX72_SDMMC0_SDMCLK         92
>> +#define AGILEX72_SDMMC1_SDMCLK         93
>> +#define AGILEX72_SDMMC0_PHY_CLK        94
>> +#define AGILEX72_SDMMC1_PHY_CLK        95
>> +#define AGILEX72_USB31_REF_CLK         96
> 
>> +#define AGILEX72_NUM_CLKS              97
> 
> No, not a binding. Drop
> 
Will drop in v2.

>> +
>> +#endif  /* __DT_BINDINGS_ALTR_AGILEX72_CLKMGR_H */
> 
> 
> Best regards,
> Krzysztof

Thanks and best regards,
Evon