Re: [PATCH v2 5/9] dt-bindings: clk: mpfs: add defines for two new clocks

[<Conor.Dooley@microchip.com>]

On 12/04/2022 11:47, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 11/04/2022 10:59, Conor Dooley wrote:
>> The RTC reference and MSSPLL were previously not documented or defined,
>> as they were unused. Add their defines to the PolarFire SoC header.
>>
>> Fixes: 2145bb687e3f ("dt-bindings: clk: microchip: Add Microchip PolarFire host binding")
>> Reviewed-by: Daire McNamara <daire.mcnamara@microchip.com>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>   include/dt-bindings/clock/microchip,mpfs-clock.h | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h
>> index 73f2a9324857..3cba46b9191f 100644
>> --- a/include/dt-bindings/clock/microchip,mpfs-clock.h
>> +++ b/include/dt-bindings/clock/microchip,mpfs-clock.h
>> @@ -1,15 +1,18 @@
>>   /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>   /*
>>    * Daire McNamara,<daire.mcnamara@microchip.com>
>> - * Copyright (C) 2020 Microchip Technology Inc.  All rights reserved.
>> + * Copyright (C) 2020-2022 Microchip Technology Inc.  All rights reserved.
>>    */
>>
>>   #ifndef _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_
>>   #define _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_
>>
>> +#define CLK_MSSPLL   34
> 
> You have some weird order here. Shouldn't it be under CLK_RTCREF?

Yeah numerically weirdly ordered - I grouped the clocks by type:
MSSPLL is a pll, CPU/AXI/AHB/RTC are all dividers & the rest are on/off
toggles. I'd've prefered to have renumbered the whole list, but that
didn't feel like a good idea.

Additionally MSSPLL is the source for CLK_{CPI,AXI,AHB} so I put it at
the top. I have no particular preference, so if you want them reordered
so that MSSPLL is under RTCREF just say the word :)

Thanks,
Conor.

> 
>> +
>>   #define CLK_CPU              0
>>   #define CLK_AXI              1
>>   #define CLK_AHB              2
>> +#define CLK_RTCREF   33
>>
>>   #define CLK_ENVM     3
>>   #define CLK_MAC0     4
> 
> 
> Best regards,
> Krzysztof