[PATCH V2 0/3] Add S4 SoC clock controller driver

[PATCH V2 0/3] Add S4 SoC clock controller driver

[Yu Tu]

1. Add clock controller driver for S4 SOC.

Yu Tu (3):
  dt-bindings: clk: meson: add S4 SoC clock controller bindings
  arm64: dts: meson: add S4 Soc clock controller in DT
  clk: meson: s4: add s4 SoC clock controller driver

V1 -> V2: Change format as discussed in the email.

Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/

 .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
 MAINTAINERS                                   |    1 +
 arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
 drivers/clk/meson/Kconfig                     |   15 +
 drivers/clk/meson/Makefile                    |    1 +
 drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
 drivers/clk/meson/s4.h                        |  296 ++
 include/dt-bindings/clock/s4-clkc.h           |  146 +
 8 files changed, 5203 insertions(+)
 create mode 100644 drivers/clk/meson/s4.c
 create mode 100644 drivers/clk/meson/s4.h
 create mode 100644 include/dt-bindings/clock/s4-clkc.h


base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Yu Tu]

Add new clock controller compatible and dt-bindings header for the
Everything-Else domain of the S4 SoC.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 .../bindings/clock/amlogic,gxbb-clkc.txt      |   1 +
 MAINTAINERS                                   |   1 +
 include/dt-bindings/clock/s4-clkc.h           | 146 ++++++++++++++++++
 3 files changed, 148 insertions(+)
 create mode 100644 include/dt-bindings/clock/s4-clkc.h

diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
index 7ccecd5c02c1..301b43dea912 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
+++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
@@ -12,6 +12,7 @@ Required Properties:
 		"amlogic,g12a-clkc" for G12A SoC.
 		"amlogic,g12b-clkc" for G12B SoC.
 		"amlogic,sm1-clkc" for SM1 SoC.
+		"amlogic,s4-clkc" for S4 SoC.
 - clocks : list of clock phandle, one for each entry clock-names.
 - clock-names : should contain the following:
   * "xtal": the platform xtal
diff --git a/MAINTAINERS b/MAINTAINERS
index c1abc53f9e91..f872d0c0c253 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
 F:	drivers/clk/meson/
 F:	include/dt-bindings/clock/gxbb*
 F:	include/dt-bindings/clock/meson*
+F:	include/dt-bindings/clock/s4-clkc.h
 
 ARM/Amlogic Meson SoC Crypto Drivers
 M:	Corentin Labbe <clabbe@baylibre.com>
diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
new file mode 100644
index 000000000000..b686c8877419
--- /dev/null
+++ b/include/dt-bindings/clock/s4-clkc.h
@@ -0,0 +1,146 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
+ * Author: Yu Tu <yu.tu@amlogic.com>
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H
+#define _DT_BINDINGS_CLOCK_S4_CLKC_H
+
+/*
+ * CLKID index values
+ */
+
+#define CLKID_FIXED_PLL			1
+#define CLKID_FCLK_DIV2			3
+#define CLKID_FCLK_DIV3			5
+#define CLKID_FCLK_DIV4			7
+#define CLKID_FCLK_DIV5			9
+#define CLKID_FCLK_DIV7			11
+#define CLKID_FCLK_DIV2P5		13
+#define CLKID_GP0_PLL			15
+#define CLKID_HIFI_PLL			17
+#define CLKID_HDMI_PLL			20
+#define CLKID_MPLL_50M			22
+#define CLKID_MPLL0			25
+#define CLKID_MPLL1			27
+#define CLKID_MPLL2			29
+#define CLKID_MPLL3			31
+#define CLKID_RTC_CLK			36
+#define CLKID_SYS_CLK_B_GATE		39
+#define CLKID_SYS_CLK_A_GATE		42
+#define CLKID_SYS_CLK			43
+#define CLKID_CECA_32K_CLKOUT		48
+#define CLKID_CECB_32K_CLKOUT		53
+#define CLKID_SC_CLK_GATE		56
+#define CLKID_12_24M_CLK_SEL		59
+#define CLKID_VID_PLL			62
+#define CLKID_VCLK			69
+#define CLKID_VCLK2			70
+#define CLKID_VCLK_DIV1			71
+#define CLKID_VCLK2_DIV1		76
+#define CLKID_VCLK_DIV2			81
+#define CLKID_VCLK_DIV4			82
+#define CLKID_VCLK_DIV6			83
+#define CLKID_VCLK_DIV12		84
+#define CLKID_VCLK2_DIV2		85
+#define CLKID_VCLK2_DIV4		86
+#define CLKID_VCLK2_DIV6		87
+#define CLKID_VCLK2_DIV12		88
+#define CLKID_CTS_ENCI			93
+#define CLKID_CTS_ENCP			94
+#define CLKID_CTS_VDAC			95
+#define CLKID_HDMI			99
+#define CLKID_TS_CLK_GATE		101
+#define CLKID_MALI_0			104
+#define CLKID_MALI_1			107
+#define CLKID_MALI			108
+#define CLKID_VDEC_P0			111
+#define CLKID_VDEC_P1			114
+#define CLKID_VDEC_SEL			115
+#define CLKID_HEVCF_P0			118
+#define CLKID_HEVCF_P1			121
+#define CLKID_HEVCF_SEL			122
+#define CLKID_VPU_0			125
+#define CLKID_VPU_1			128
+#define CLKID_VPU			129
+#define CLKID_VPU_CLKB_TMP		132
+#define CLKID_VPU_CLKB			134
+#define CLKID_VPU_CLKC_P0		137
+#define CLKID_VPU_CLKC_P1		140
+#define CLKID_VPU_CLKC_SEL		141
+#define CLKID_VAPB_0			144
+#define CLKID_VAPB_1			147
+#define CLKID_VAPB			148
+#define CLKID_GE2D			149
+#define CLKID_VDIN_MEAS_GATE		152
+#define CLKID_SD_EMMC_C_CLK		155
+#define CLKID_SD_EMMC_A_CLK		158
+#define CLKID_SD_EMMC_B_CLK		161
+#define CLKID_SPICC0_GATE		164
+#define CLKID_PWM_A_GATE		167
+#define CLKID_PWM_B_GATE		170
+#define CLKID_PWM_C_GATE		173
+#define CLKID_PWM_D_GATE		176
+#define CLKID_PWM_E_GATE		179
+#define CLKID_PWM_F_GATE		182
+#define CLKID_PWM_G_GATE		185
+#define CLKID_PWM_H_GATE		188
+#define CLKID_PWM_I_GATE		191
+#define CLKID_PWM_J_GATE		194
+#define CLKID_SARADC_GATE		197
+#define CLKID_GEN_GATE			200
+#define CLKID_DDR			201
+#define CLKID_DOS			202
+#define CLKID_ETHPHY			203
+#define CLKID_MALI_GATE			204
+#define CLKID_AOCPU			205
+#define CLKID_AUCPU			206
+#define CLKID_CEC			207
+#define CLKID_SD_EMMC_A			208
+#define CLKID_SD_EMMC_B			209
+#define CLKID_NAND			210
+#define CLKID_SMARTCARD			211
+#define CLKID_ACODEC			212
+#define CLKID_SPIFC			213
+#define CLKID_MSR_CLK			214
+#define CLKID_IR_CTRL			215
+#define CLKID_AUDIO			216
+#define CLKID_ETH			217
+#define CLKID_UART_A			218
+#define CLKID_UART_B			219
+#define CLKID_UART_C			220
+#define CLKID_UART_D			221
+#define CLKID_UART_E			222
+#define CLKID_AIFIFO			223
+#define CLKID_TS_DDR			224
+#define CLKID_TS_PLL			225
+#define CLKID_G2D			226
+#define CLKID_SPICC0			227
+#define CLKID_SPICC1			228
+#define CLKID_USB			229
+#define CLKID_I2C_M_A			230
+#define CLKID_I2C_M_B			231
+#define CLKID_I2C_M_C			232
+#define CLKID_I2C_M_D			233
+#define CLKID_I2C_M_E			234
+#define CLKID_HDMITX_APB		235
+#define CLKID_I2C_S_A			236
+#define CLKID_USB1_TO_DDR		237
+#define CLKID_HDCP22			238
+#define CLKID_MMC_APB			239
+#define CLKID_RSA			240
+#define CLKID_CPU_DEBUG			241
+#define CLKID_VPU_INTR			242
+#define CLKID_DEMOD			243
+#define CLKID_SAR_ADC			244
+#define CLKID_GIC			245
+#define CLKID_PWM_AB			246
+#define CLKID_PWM_CD			247
+#define CLKID_PWM_EF			248
+#define CLKID_PWM_GH			249
+#define CLKID_PWM_IJ			250
+#define CLKID_HDCP22_ESMCLK_GATE	253
+#define CLKID_HDCP22_SKPCLK_GATE	256
+
+#endif /* _DT_BINDINGS_CLOCK_S4_CLKC_H */
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 2/3] arm64: dts: meson: add S4 Soc clock controller in DT

[Yu Tu]

Added information about the S4 SOC Clock controller in DT.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
index ff213618a598..3add7fcaffa9 100644
--- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
@@ -6,6 +6,7 @@
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/clock/s4-clkc.h>
 
 / {
 	cpus {
@@ -118,6 +119,16 @@ gpio_intc: interrupt-controller@4080 {
 					<10 11 12 13 14 15 16 17 18 19 20 21>;
 			};
 
+			clkc: clock-controller {
+				compatible = "amlogic,s4-clkc";
+				reg = <0x0 0x0 0x0 0x49c>,
+				      <0x0 0x8000 0x0 0x348>;
+				reg-names = "basic", "pll";
+				#clock-cells = <1>;
+				clocks = <&xtal>;
+				clock-names = "xtal";
+			};
+
 			uart_B: serial@7a000 {
 				compatible = "amlogic,meson-s4-uart",
 					     "amlogic,meson-ao-uart";
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/3] Add S4 SoC clock controller driver

[Jerome Brunet]

On Thu 28 Jul 2022 at 13:41, Yu Tu <yu.tu@amlogic.com> wrote:

> 1. Add clock controller driver for S4 SOC.
>
> Yu Tu (3):
>   dt-bindings: clk: meson: add S4 SoC clock controller bindings
>   arm64: dts: meson: add S4 Soc clock controller in DT
>   clk: meson: s4: add s4 SoC clock controller driver
>
> V1 -> V2: Change format as discussed in the email.
>
> Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/
>
>  .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
>  MAINTAINERS                                   |    1 +
>  arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
>  drivers/clk/meson/Kconfig                     |   15 +
>  drivers/clk/meson/Makefile                    |    1 +
>  drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
>  drivers/clk/meson/s4.h                        |  296 ++
>  include/dt-bindings/clock/s4-clkc.h           |  146 +
>  8 files changed, 5203 insertions(+)
>  create mode 100644 drivers/clk/meson/s4.c
>  create mode 100644 drivers/clk/meson/s4.h
>  create mode 100644 include/dt-bindings/clock/s4-clkc.h
>
>
> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6

Please don't post until you have addressed *ALL* the comments from the
previous version.

At first glance, I can see that this is still a single driver for
what is obviously 2 controllers with 2 register spaces. Simple comments
like the "<< 2" in the register declaration have not been addressed either.

Seeing that, I have not reviewed this version further.
I won't until all the comments from v1 are either addressed or answered.

Regards
Jerome

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/3] Add S4 SoC clock controller driver

[Yu Tu]

Hi JB，

On 2022/7/28 15:08, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Thu 28 Jul 2022 at 13:41, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> 1. Add clock controller driver for S4 SOC.
>>
>> Yu Tu (3):
>>    dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>    arm64: dts: meson: add S4 Soc clock controller in DT
>>    clk: meson: s4: add s4 SoC clock controller driver
>>
>> V1 -> V2: Change format as discussed in the email.
>>
>> Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/
>>
>>   .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
>>   MAINTAINERS                                   |    1 +
>>   arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
>>   drivers/clk/meson/Kconfig                     |   15 +
>>   drivers/clk/meson/Makefile                    |    1 +
>>   drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
>>   drivers/clk/meson/s4.h                        |  296 ++
>>   include/dt-bindings/clock/s4-clkc.h           |  146 +
>>   8 files changed, 5203 insertions(+)
>>   create mode 100644 drivers/clk/meson/s4.c
>>   create mode 100644 drivers/clk/meson/s4.h
>>   create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>
>>
>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
> 
> Please don't post until you have addressed *ALL* the comments from the
> previous version.
The last email asked you to adopt A1 method, but you did not reply?

> 
> At first glance, I can see that this is still a single driver for
> what is obviously 2 controllers with 2 register spaces. Simple comments
> like the "<< 2" in the register declaration have not been addressed either.
I understand that this should be a controller, just two address 
descriptions. One is the various PLL registers and one is the clock for 
the peripherals. And PLL is to provide a clock source for various 
peripheral clocks. So a clock controller is reasonable. I think you got 
it wrong.

Ok, if you insist on using two clock controllers,, please provide your 
the reason and example code?

> 
> Seeing that, I have not reviewed this version further.
> I won't until all the comments from v1 are either addressed or answer
> 
> Regards
> Jerome
> 
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/3] Add S4 SoC clock controller driver

[Yu Tu]

Hi JB,
	We suspect that you are not looking carefully. You only see two base 
addresses for controllers, so you think you should use two clock 
controllers. But you're missing their direct connection.

On 2022/7/28 16:06, Yu Tu wrote:
> Hi JB，
> 
> On 2022/7/28 15:08, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>>
>> On Thu 28 Jul 2022 at 13:41, Yu Tu <yu.tu@amlogic.com> wrote:
>>
>>> 1. Add clock controller driver for S4 SOC.
>>>
>>> Yu Tu (3):
>>>    dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>    arm64: dts: meson: add S4 Soc clock controller in DT
>>>    clk: meson: s4: add s4 SoC clock controller driver
>>>
>>> V1 -> V2: Change format as discussed in the email.
>>>
>>> Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/ 
>>>
>>>
>>>   .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
>>>   MAINTAINERS                                   |    1 +
>>>   arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
>>>   drivers/clk/meson/Kconfig                     |   15 +
>>>   drivers/clk/meson/Makefile                    |    1 +
>>>   drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
>>>   drivers/clk/meson/s4.h                        |  296 ++
>>>   include/dt-bindings/clock/s4-clkc.h           |  146 +
>>>   8 files changed, 5203 insertions(+)
>>>   create mode 100644 drivers/clk/meson/s4.c
>>>   create mode 100644 drivers/clk/meson/s4.h
>>>   create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>
>>>
>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>
>> Please don't post until you have addressed *ALL* the comments from the
>> previous version.
> The last email asked you to adopt A1 method, but you did not reply?
> 
>>
>> At first glance, I can see that this is still a single driver for
>> what is obviously 2 controllers with 2 register spaces. Simple comments
>> like the "<< 2" in the register declaration have not been addressed 
>> either.
> I understand that this should be a controller, just two address 
> descriptions. One is the various PLL registers and one is the clock for 
> the peripherals. And PLL is to provide a clock source for various 
> peripheral clocks. So a clock controller is reasonable. I think you got 
> it wrong.
> 
> Ok, if you insist on using two clock controllers,, please provide your 
> the reason and example code?
> 
>>
>> Seeing that, I have not reviewed this version further.
>> I won't until all the comments from v1 are either addressed or answer
>>
>> Regards
>> Jerome
>>
>> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/3] Add S4 SoC clock controller driver

[Jerome Brunet]

On Thu 28 Jul 2022 at 16:06, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi JB，
>
> On 2022/7/28 15:08, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> 
>> On Thu 28 Jul 2022 at 13:41, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> 1. Add clock controller driver for S4 SOC.
>>>
>>> Yu Tu (3):
>>>    dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>    arm64: dts: meson: add S4 Soc clock controller in DT
>>>    clk: meson: s4: add s4 SoC clock controller driver
>>>
>>> V1 -> V2: Change format as discussed in the email.
>>>
>>> Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/
>>>
>>>   .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
>>>   MAINTAINERS                                   |    1 +
>>>   arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
>>>   drivers/clk/meson/Kconfig                     |   15 +
>>>   drivers/clk/meson/Makefile                    |    1 +
>>>   drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
>>>   drivers/clk/meson/s4.h                        |  296 ++
>>>   include/dt-bindings/clock/s4-clkc.h           |  146 +
>>>   8 files changed, 5203 insertions(+)
>>>   create mode 100644 drivers/clk/meson/s4.c
>>>   create mode 100644 drivers/clk/meson/s4.h
>>>   create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>
>>>
>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>> Please don't post until you have addressed *ALL* the comments from the
>> previous version.
> The last email asked you to adopt A1 method, but you did not reply?
>
>> At first glance, I can see that this is still a single driver for
>> what is obviously 2 controllers with 2 register spaces. Simple comments
>> like the "<< 2" in the register declaration have not been addressed either.
> I understand that this should be a controller, just two address
> descriptions. One is the various PLL registers and one is the clock for 
> the peripherals. And PLL is to provide a clock source for various
> peripheral clocks. So a clock controller is reasonable. I think you got 
> it wrong.

I don't think I do. This looks exactly like the A1.
The post of that controller are still in the  archive and I am sure your
colleagues can give you the history.

You clearly have register regions providing clock, separated by
0x8000. Claiming that as one big region is bad design.

There has been several remarks about using a big syscon on V1,
unaddressed too.

CCF has everything necessary in place to handle each register region
separately, properly and pass clock around.

You can handle it as a single controller, claiming the two regions
individually but:
# 1 - handling 2 different regmaps in the controller is going to be
      bigger mess than you think
# 2 - I am far from convinced there is any reason to do so


>
> Ok, if you insist on using two clock controllers,, please provide your the
> reason and example code?
>
>> Seeing that, I have not reviewed this version further.
>> I won't until all the comments from v1 are either addressed or answer
>> Regards
>> Jerome
>> .


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Krzysztof Kozlowski]

On 28/07/2022 07:42, Yu Tu wrote:
> Add new clock controller compatible and dt-bindings header for the
> Everything-Else domain of the S4 SoC.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>



> diff --git a/MAINTAINERS b/MAINTAINERS
> index c1abc53f9e91..f872d0c0c253 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>  F:	drivers/clk/meson/
>  F:	include/dt-bindings/clock/gxbb*
>  F:	include/dt-bindings/clock/meson*
> +F:	include/dt-bindings/clock/s4-clkc.h
>  
>  ARM/Amlogic Meson SoC Crypto Drivers
>  M:	Corentin Labbe <clabbe@baylibre.com>
> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
> new file mode 100644
> index 000000000000..b686c8877419
> --- /dev/null
> +++ b/include/dt-bindings/clock/s4-clkc.h

Filename with vendor prefix, so:
amlogic,s4-clkc.h

> @@ -0,0 +1,146 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
> + * Author: Yu Tu <yu.tu@amlogic.com>
> + */
> +
> +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H
> +#define _DT_BINDINGS_CLOCK_S4_CLKC_H
> +
> +/*
> + * CLKID index values
> + */
> +
> +#define CLKID_FIXED_PLL			1
> +#define CLKID_FCLK_DIV2			3
> +#define CLKID_FCLK_DIV3			5
> +#define CLKID_FCLK_DIV4			7
> +#define CLKID_FCLK_DIV5			9
> +#define CLKID_FCLK_DIV7			11

Why these aren't continuous? IDs are expected to be incremented by 1.

> +
> +#endif /* _DT_BINDINGS_CLOCK_S4_CLKC_H */


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Jerome Brunet]

On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 28/07/2022 07:42, Yu Tu wrote:
>> Add new clock controller compatible and dt-bindings header for the
>> Everything-Else domain of the S4 SoC.
>> 
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>
>
>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c1abc53f9e91..f872d0c0c253 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>>  F:	drivers/clk/meson/
>>  F:	include/dt-bindings/clock/gxbb*
>>  F:	include/dt-bindings/clock/meson*
>> +F:	include/dt-bindings/clock/s4-clkc.h
>>  
>>  ARM/Amlogic Meson SoC Crypto Drivers
>>  M:	Corentin Labbe <clabbe@baylibre.com>
>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
>> new file mode 100644
>> index 000000000000..b686c8877419
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/s4-clkc.h
>
> Filename with vendor prefix, so:
> amlogic,s4-clkc.h
>
>> @@ -0,0 +1,146 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
>> + * Author: Yu Tu <yu.tu@amlogic.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H
>> +#define _DT_BINDINGS_CLOCK_S4_CLKC_H
>> +
>> +/*
>> + * CLKID index values
>> + */
>> +
>> +#define CLKID_FIXED_PLL			1
>> +#define CLKID_FCLK_DIV2			3
>> +#define CLKID_FCLK_DIV3			5
>> +#define CLKID_FCLK_DIV4			7
>> +#define CLKID_FCLK_DIV5			9
>> +#define CLKID_FCLK_DIV7			11
>
> Why these aren't continuous? IDs are expected to be incremented by 1.
>

All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
For example, with composite 'mux / div / gate' assembly, we usually need
only the leaf.

Same has been done for the other AML controllers:
For ex:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/3] Add S4 SoC clock controller driver

[Yu Tu]

Hi Jerome,
	Thanks for your reply and explanation.

On 2022/7/28 16:27, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Thu 28 Jul 2022 at 16:06, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi JB，
>>
>> On 2022/7/28 15:08, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Thu 28 Jul 2022 at 13:41, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> 1. Add clock controller driver for S4 SOC.
>>>>
>>>> Yu Tu (3):
>>>>     dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>>     arm64: dts: meson: add S4 Soc clock controller in DT
>>>>     clk: meson: s4: add s4 SoC clock controller driver
>>>>
>>>> V1 -> V2: Change format as discussed in the email.
>>>>
>>>> Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/
>>>>
>>>>    .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
>>>>    MAINTAINERS                                   |    1 +
>>>>    arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
>>>>    drivers/clk/meson/Kconfig                     |   15 +
>>>>    drivers/clk/meson/Makefile                    |    1 +
>>>>    drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
>>>>    drivers/clk/meson/s4.h                        |  296 ++
>>>>    include/dt-bindings/clock/s4-clkc.h           |  146 +
>>>>    8 files changed, 5203 insertions(+)
>>>>    create mode 100644 drivers/clk/meson/s4.c
>>>>    create mode 100644 drivers/clk/meson/s4.h
>>>>    create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>>
>>>>
>>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>> Please don't post until you have addressed *ALL* the comments from the
>>> previous version.
>> The last email asked you to adopt A1 method, but you did not reply?
>>
>>> At first glance, I can see that this is still a single driver for
>>> what is obviously 2 controllers with 2 register spaces. Simple comments
>>> like the "<< 2" in the register declaration have not been addressed either.
>> I understand that this should be a controller, just two address
>> descriptions. One is the various PLL registers and one is the clock for
>> the peripherals. And PLL is to provide a clock source for various
>> peripheral clocks. So a clock controller is reasonable. I think you got
>> it wrong.
> 
> I don't think I do. This looks exactly like the A1.
> The post of that controller are still in the  archive and I am sure your
> colleagues can give you the history.
> 
> You clearly have register regions providing clock, separated by
> 0x8000. Claiming that as one big region is bad design.
> 
> There has been several remarks about using a big syscon on V1,
> unaddressed too.
> 
> CCF has everything necessary in place to handle each register region
> separately, properly and pass clock around.
> 
> You can handle it as a single controller, claiming the two regions
> individually but:
> # 1 - handling 2 different regmaps in the controller is going to be
>        bigger mess than you think
> # 2 - I am far from convinced there is any reason to do so
> 
It makes sense, as you say, to separate the two controllers. But I think 
the only thing that was forced apart was that the digital designers 
didn't put these registers together when they were designing the chips.

I'm going to separate the two controllers like you said.

> 
>>
>> Ok, if you insist on using two clock controllers,, please provide your the
>> reason and example code?
>>
>>> Seeing that, I have not reviewed this version further.
>>> I won't until all the comments from v1 are either addressed or answer
>>> Regards
>>> Jerome
>>> .
> 
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Krzysztof Kozlowski]

On 28/07/2022 10:50, Jerome Brunet wrote:
> 
> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 28/07/2022 07:42, Yu Tu wrote:
>>> Add new clock controller compatible and dt-bindings header for the
>>> Everything-Else domain of the S4 SoC.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>
>>
>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index c1abc53f9e91..f872d0c0c253 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>>>  F:	drivers/clk/meson/
>>>  F:	include/dt-bindings/clock/gxbb*
>>>  F:	include/dt-bindings/clock/meson*
>>> +F:	include/dt-bindings/clock/s4-clkc.h
>>>  
>>>  ARM/Amlogic Meson SoC Crypto Drivers
>>>  M:	Corentin Labbe <clabbe@baylibre.com>
>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
>>> new file mode 100644
>>> index 000000000000..b686c8877419
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/s4-clkc.h
>>
>> Filename with vendor prefix, so:
>> amlogic,s4-clkc.h
>>
>>> @@ -0,0 +1,146 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>>> +/*
>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
>>> + * Author: Yu Tu <yu.tu@amlogic.com>
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H
>>> +#define _DT_BINDINGS_CLOCK_S4_CLKC_H
>>> +
>>> +/*
>>> + * CLKID index values
>>> + */
>>> +
>>> +#define CLKID_FIXED_PLL			1
>>> +#define CLKID_FCLK_DIV2			3
>>> +#define CLKID_FCLK_DIV3			5
>>> +#define CLKID_FCLK_DIV4			7
>>> +#define CLKID_FCLK_DIV5			9
>>> +#define CLKID_FCLK_DIV7			11
>>
>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>
> 
> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
> For example, with composite 'mux / div / gate' assembly, we usually need
> only the leaf.

I understand you do not expose them all, but that is not the reason to
increment ID by 2 or 3... Otherwise these are not IDs and you are not
expected to put register offsets into the bindings (you do not bindings
in such case).


> Same has been done for the other AML controllers:
> For ex:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h

This cannot be fixed now, but it is very poor argument. Like saying "we
had a bug in other driver, so we implemented the bug here as well".

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/3] Add S4 SoC clock controller driver

[Jerome Brunet]

On Thu 28 Jul 2022 at 16:55, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Jerome,
> 	Thanks for your reply and explanation.
>
> On 2022/7/28 16:27, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> 
>> On Thu 28 Jul 2022 at 16:06, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> Hi JB，
>>>
>>> On 2022/7/28 15:08, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On Thu 28 Jul 2022 at 13:41, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>
>>>>> 1. Add clock controller driver for S4 SOC.
>>>>>
>>>>> Yu Tu (3):
>>>>>     dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>>>     arm64: dts: meson: add S4 Soc clock controller in DT
>>>>>     clk: meson: s4: add s4 SoC clock controller driver
>>>>>
>>>>> V1 -> V2: Change format as discussed in the email.
>>>>>
>>>>> Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/
>>>>>
>>>>>    .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
>>>>>    MAINTAINERS                                   |    1 +
>>>>>    arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
>>>>>    drivers/clk/meson/Kconfig                     |   15 +
>>>>>    drivers/clk/meson/Makefile                    |    1 +
>>>>>    drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
>>>>>    drivers/clk/meson/s4.h                        |  296 ++
>>>>>    include/dt-bindings/clock/s4-clkc.h           |  146 +
>>>>>    8 files changed, 5203 insertions(+)
>>>>>    create mode 100644 drivers/clk/meson/s4.c
>>>>>    create mode 100644 drivers/clk/meson/s4.h
>>>>>    create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>>>
>>>>>
>>>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>>> Please don't post until you have addressed *ALL* the comments from the
>>>> previous version.
>>> The last email asked you to adopt A1 method, but you did not reply?
>>>
>>>> At first glance, I can see that this is still a single driver for
>>>> what is obviously 2 controllers with 2 register spaces. Simple comments
>>>> like the "<< 2" in the register declaration have not been addressed either.
>>> I understand that this should be a controller, just two address
>>> descriptions. One is the various PLL registers and one is the clock for
>>> the peripherals. And PLL is to provide a clock source for various
>>> peripheral clocks. So a clock controller is reasonable. I think you got
>>> it wrong.
>> I don't think I do. This looks exactly like the A1.
>> The post of that controller are still in the  archive and I am sure your
>> colleagues can give you the history.
>> You clearly have register regions providing clock, separated by
>> 0x8000. Claiming that as one big region is bad design.
>> There has been several remarks about using a big syscon on V1,
>> unaddressed too.
>> CCF has everything necessary in place to handle each register region
>> separately, properly and pass clock around.
>> You can handle it as a single controller, claiming the two regions
>> individually but:
>> # 1 - handling 2 different regmaps in the controller is going to be
>>        bigger mess than you think
>> # 2 - I am far from convinced there is any reason to do so
>> 
> It makes sense, as you say, to separate the two controllers. But I think
> the only thing that was forced apart was that the digital designers 
> didn't put these registers together when they were designing the chips.
>

One controller is providing all the base PLLs
The other is providing most (if not all) the devices clocks.
This does not look like coincidence or mistake to me.

> I'm going to separate the two controllers like you said.
>
>> 
>>>
>>> Ok, if you insist on using two clock controllers,, please provide your the
>>> reason and example code?
>>>
>>>> Seeing that, I have not reviewed this version further.
>>>> I won't until all the comments from v1 are either addressed or answer
>>>> Regards
>>>> Jerome
>>>> .
>> .


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Jerome Brunet]

On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 28/07/2022 10:50, Jerome Brunet wrote:
>> 
>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>> 
>>> On 28/07/2022 07:42, Yu Tu wrote:
[...]
>>>> +/*
>>>> + * CLKID index values
>>>> + */
>>>> +
>>>> +#define CLKID_FIXED_PLL			1
>>>> +#define CLKID_FCLK_DIV2			3
>>>> +#define CLKID_FCLK_DIV3			5
>>>> +#define CLKID_FCLK_DIV4			7
>>>> +#define CLKID_FCLK_DIV5			9
>>>> +#define CLKID_FCLK_DIV7			11
>>>
>>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>>
>> 
>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
>> For example, with composite 'mux / div / gate' assembly, we usually need
>> only the leaf.
>
> I understand you do not expose them all, but that is not the reason to
> increment ID by 2 or 3... Otherwise these are not IDs and you are not
> expected to put register offsets into the bindings (you do not bindings
> in such case).

Why is it not an IDs if it not continuous in the bindings ?

If there is technical reason, we'll probably end up exposing everything. It
would not be a dramatic change. I asked for this over v1 because we have
done that is the past and I think it makes sense.

I'm happy to be convinced to do things differently. Just looking for the
technical reason that require contiuous exposed IDs.

The other IDs exists, but we do not expose them as bindings.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125

>
>
>> Same has been done for the other AML controllers:
>> For ex:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h
>
> This cannot be fixed now, but it is very poor argument. Like saying "we
> had a bug in other driver, so we implemented the bug here as well".

I agree, "done before" is not a good argument. I was trying to provide a
better picutre. I'm just surprised to have this new requirement that IDs
have to be incremented by 1 (in the bindings) and I'd like to understand
why what we had done could be considered a bug now.

For example the simple-reset driver compute the reset offset from the IDs:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-simple.c
There might be holes in the IDs if not all bits have reset maps.
I don't think that would be a bug either.

>
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Krzysztof Kozlowski]

On 28/07/2022 11:09, Jerome Brunet wrote:
> 
> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 28/07/2022 10:50, Jerome Brunet wrote:
>>>
>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 28/07/2022 07:42, Yu Tu wrote:
> [...]
>>>>> +/*
>>>>> + * CLKID index values
>>>>> + */
>>>>> +
>>>>> +#define CLKID_FIXED_PLL			1
>>>>> +#define CLKID_FCLK_DIV2			3
>>>>> +#define CLKID_FCLK_DIV3			5
>>>>> +#define CLKID_FCLK_DIV4			7
>>>>> +#define CLKID_FCLK_DIV5			9
>>>>> +#define CLKID_FCLK_DIV7			11
>>>>
>>>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>>>
>>>
>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
>>> For example, with composite 'mux / div / gate' assembly, we usually need
>>> only the leaf.
>>
>> I understand you do not expose them all, but that is not the reason to
>> increment ID by 2 or 3... Otherwise these are not IDs and you are not
>> expected to put register offsets into the bindings (you do not bindings
>> in such case).
> 
> Why is it not an IDs if it not continuous in the bindings ?
> 
> If there is technical reason, we'll probably end up exposing everything. It
> would not be a dramatic change. I asked for this over v1 because we have
> done that is the past and I think it makes sense.
> 
> I'm happy to be convinced to do things differently. Just looking for the
> technical reason that require contiuous exposed IDs.
> 
> The other IDs exists, but we do not expose them as bindings.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125

https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/

https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/

https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/

The IDs are abstract numbers, where the number does not matter because
it is not tied to driver implementation or device programming model. The
driver maps ID to respective clock.

Using some meaningful numbers as these IDs, means you tied bindings to
your implementation and any change in implementation requires change in
the bindings. This contradicts the idea of bindings.

> 
>>
>>
>>> Same has been done for the other AML controllers:
>>> For ex:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h
>>
>> This cannot be fixed now, but it is very poor argument. Like saying "we
>> had a bug in other driver, so we implemented the bug here as well".
> 
> I agree, "done before" is not a good argument. I was trying to provide a
> better picutre. I'm just surprised to have this new requirement that IDs
> have to be incremented by 1 (in the bindings) and I'd like to understand
> why what we had done could be considered a bug now.

It was always, just no one ever enforced it. And almost all clock and
reset providers follow it. There are just literally few exceptions.

> For example the simple-reset driver compute the reset offset from the IDs:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-simple.c

This is one of the exceptions where it actually made sense, but I would
argue it still contradicts the bindings. You have now binding which is
tied to both Linux implementation and to device programming model.

However fixing it would require creating huge mapping tables for each
SoC, so obviously this exception is quite reasonable.

Clock drivers require tables and translation anyway. Almost all clock
drivers did it, so such exception is not justified.

> There might be holes in the IDs if not all bits have reset maps.
> I don't think that would be a bug either.

Bug was of course highly exaggerated example. :)

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/3] Add S4 SoC clock controller driver

[Yu Tu]

On 2022/7/28 17:03, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Thu 28 Jul 2022 at 16:55, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi Jerome,
>> 	Thanks for your reply and explanation.
>>
>> On 2022/7/28 16:27, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Thu 28 Jul 2022 at 16:06, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Hi JB，
>>>>
>>>> On 2022/7/28 15:08, Jerome Brunet wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> On Thu 28 Jul 2022 at 13:41, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>
>>>>>> 1. Add clock controller driver for S4 SOC.
>>>>>>
>>>>>> Yu Tu (3):
>>>>>>      dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>>>>      arm64: dts: meson: add S4 Soc clock controller in DT
>>>>>>      clk: meson: s4: add s4 SoC clock controller driver
>>>>>>
>>>>>> V1 -> V2: Change format as discussed in the email.
>>>>>>
>>>>>> Link:https://lore.kernel.org/linux-amlogic/20220708062757.3662-1-yu.tu@amlogic.com/
>>>>>>
>>>>>>     .../bindings/clock/amlogic,gxbb-clkc.txt      |    1 +
>>>>>>     MAINTAINERS                                   |    1 +
>>>>>>     arch/arm64/boot/dts/amlogic/meson-s4.dtsi     |   11 +
>>>>>>     drivers/clk/meson/Kconfig                     |   15 +
>>>>>>     drivers/clk/meson/Makefile                    |    1 +
>>>>>>     drivers/clk/meson/s4.c                        | 4732 +++++++++++++++++
>>>>>>     drivers/clk/meson/s4.h                        |  296 ++
>>>>>>     include/dt-bindings/clock/s4-clkc.h           |  146 +
>>>>>>     8 files changed, 5203 insertions(+)
>>>>>>     create mode 100644 drivers/clk/meson/s4.c
>>>>>>     create mode 100644 drivers/clk/meson/s4.h
>>>>>>     create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>>>>
>>>>>>
>>>>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>>>> Please don't post until you have addressed *ALL* the comments from the
>>>>> previous version.
>>>> The last email asked you to adopt A1 method, but you did not reply?
>>>>
>>>>> At first glance, I can see that this is still a single driver for
>>>>> what is obviously 2 controllers with 2 register spaces. Simple comments
>>>>> like the "<< 2" in the register declaration have not been addressed either.
>>>> I understand that this should be a controller, just two address
>>>> descriptions. One is the various PLL registers and one is the clock for
>>>> the peripherals. And PLL is to provide a clock source for various
>>>> peripheral clocks. So a clock controller is reasonable. I think you got
>>>> it wrong.
>>> I don't think I do. This looks exactly like the A1.
>>> The post of that controller are still in the  archive and I am sure your
>>> colleagues can give you the history.
>>> You clearly have register regions providing clock, separated by
>>> 0x8000. Claiming that as one big region is bad design.
>>> There has been several remarks about using a big syscon on V1,
>>> unaddressed too.
>>> CCF has everything necessary in place to handle each register region
>>> separately, properly and pass clock around.
>>> You can handle it as a single controller, claiming the two regions
>>> individually but:
>>> # 1 - handling 2 different regmaps in the controller is going to be
>>>         bigger mess than you think
>>> # 2 - I am far from convinced there is any reason to do so
>>>
>> It makes sense, as you say, to separate the two controllers. But I think
>> the only thing that was forced apart was that the digital designers
>> didn't put these registers together when they were designing the chips.
>>
> 
> One controller is providing all the base PLLs
> The other is providing most (if not all) the devices clocks.
> This does not look like coincidence or mistake to me.
Thanks for your reply. Looks like I got it wrong. However, I will talk 
to the chip designer about whether it is possible to put these registers 
in the later chip so that it may be easier for our software to process.

> 
>> I'm going to separate the two controllers like you said.
>>
>>>
>>>>
>>>> Ok, if you insist on using two clock controllers,, please provide your the
>>>> reason and example code?
>>>>
>>>>> Seeing that, I have not reviewed this version further.
>>>>> I won't until all the comments from v1 are either addressed or answer
>>>>> Regards
>>>>> Jerome
>>>>> .
>>> .
> 
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Jerome Brunet]

On Thu 28 Jul 2022 at 11:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 28/07/2022 11:09, Jerome Brunet wrote:
>> 
>> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>> 
>>> On 28/07/2022 10:50, Jerome Brunet wrote:
>>>>
>>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>>> On 28/07/2022 07:42, Yu Tu wrote:
>> [...]
>>>>>> +/*
>>>>>> + * CLKID index values
>>>>>> + */
>>>>>> +
>>>>>> +#define CLKID_FIXED_PLL			1
>>>>>> +#define CLKID_FCLK_DIV2			3
>>>>>> +#define CLKID_FCLK_DIV3			5
>>>>>> +#define CLKID_FCLK_DIV4			7
>>>>>> +#define CLKID_FCLK_DIV5			9
>>>>>> +#define CLKID_FCLK_DIV7			11
>>>>>
>>>>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>>>>
>>>>
>>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
>>>> For example, with composite 'mux / div / gate' assembly, we usually need
>>>> only the leaf.
>>>
>>> I understand you do not expose them all, but that is not the reason to
>>> increment ID by 2 or 3... Otherwise these are not IDs and you are not
>>> expected to put register offsets into the bindings (you do not bindings
>>> in such case).
>> 
>> Why is it not an IDs if it not continuous in the bindings ?
>> 
>> If there is technical reason, we'll probably end up exposing everything. It
>> would not be a dramatic change. I asked for this over v1 because we have
>> done that is the past and I think it makes sense.
>> 
>> I'm happy to be convinced to do things differently. Just looking for the
>> technical reason that require contiuous exposed IDs.
>> 
>> The other IDs exists, but we do not expose them as bindings.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125
>
> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
>
> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>
> https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>
> The IDs are abstract numbers, where the number does not matter because
> it is not tied to driver implementation or device programming model. The
> driver maps ID to respective clock.
>
> Using some meaningful numbers as these IDs, means you tied bindings to
> your implementation and any change in implementation requires change in
> the bindings. This contradicts the idea of bindings.
>

I totally agree. Bindings ID are abstract numbers.
We do follow that. We even document it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n118

It is just a choice to not expose some IDs.
It is not tied to the implementation at all.
I think we actually follow the rules and the idea behind it.

We can expose then all If you still think what we are doing is not appropriate.

I'd like things to be consistent though. So if the decision is to
expose everything, I'll probably end up doing the same for the old SoCs.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Yu Tu]

Hi Krzysztof,
	Thanks for your reply.

On 2022/7/28 16:41, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
> 
> On 28/07/2022 07:42, Yu Tu wrote:
>> Add new clock controller compatible and dt-bindings header for the
>> Everything-Else domain of the S4 SoC.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> 
> 
> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c1abc53f9e91..f872d0c0c253 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>>   F:	drivers/clk/meson/
>>   F:	include/dt-bindings/clock/gxbb*
>>   F:	include/dt-bindings/clock/meson*
>> +F:	include/dt-bindings/clock/s4-clkc.h
>>   
>>   ARM/Amlogic Meson SoC Crypto Drivers
>>   M:	Corentin Labbe <clabbe@baylibre.com>
>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
>> new file mode 100644
>> index 000000000000..b686c8877419
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/s4-clkc.h
> 
> Filename with vendor prefix, so:
> amlogic,s4-clkc.h
It's fine with me. It's mainly Jerome's opinion.

> 
>> @@ -0,0 +1,146 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
>> + * Author: Yu Tu <yu.tu@amlogic.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H
>> +#define _DT_BINDINGS_CLOCK_S4_CLKC_H
>> +
>> +/*
>> + * CLKID index values
>> + */
>> +
>> +#define CLKID_FIXED_PLL			1
>> +#define CLKID_FCLK_DIV2			3
>> +#define CLKID_FCLK_DIV3			5
>> +#define CLKID_FCLK_DIV4			7
>> +#define CLKID_FCLK_DIV5			9
>> +#define CLKID_FCLK_DIV7			11
> 
> Why these aren't continuous? IDs are expected to be incremented by 1.
> 
>> +
>> +#endif /* _DT_BINDINGS_CLOCK_S4_CLKC_H */
> 
> 
> Best regards,
> Krzysztof
> 
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Krzysztof Kozlowski]

On 28/07/2022 11:54, Jerome Brunet wrote:
> 
> On Thu 28 Jul 2022 at 11:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 28/07/2022 11:09, Jerome Brunet wrote:
>>>
>>> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 28/07/2022 10:50, Jerome Brunet wrote:
>>>>>
>>>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>>> On 28/07/2022 07:42, Yu Tu wrote:
>>> [...]
>>>>>>> +/*
>>>>>>> + * CLKID index values
>>>>>>> + */
>>>>>>> +
>>>>>>> +#define CLKID_FIXED_PLL			1
>>>>>>> +#define CLKID_FCLK_DIV2			3
>>>>>>> +#define CLKID_FCLK_DIV3			5
>>>>>>> +#define CLKID_FCLK_DIV4			7
>>>>>>> +#define CLKID_FCLK_DIV5			9
>>>>>>> +#define CLKID_FCLK_DIV7			11
>>>>>>
>>>>>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>>>>>
>>>>>
>>>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
>>>>> For example, with composite 'mux / div / gate' assembly, we usually need
>>>>> only the leaf.
>>>>
>>>> I understand you do not expose them all, but that is not the reason to
>>>> increment ID by 2 or 3... Otherwise these are not IDs and you are not
>>>> expected to put register offsets into the bindings (you do not bindings
>>>> in such case).
>>>
>>> Why is it not an IDs if it not continuous in the bindings ?
>>>
>>> If there is technical reason, we'll probably end up exposing everything. It
>>> would not be a dramatic change. I asked for this over v1 because we have
>>> done that is the past and I think it makes sense.
>>>
>>> I'm happy to be convinced to do things differently. Just looking for the
>>> technical reason that require contiuous exposed IDs.
>>>
>>> The other IDs exists, but we do not expose them as bindings.
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125
>>
>> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
>>
>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>>
>> https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>>
>> The IDs are abstract numbers, where the number does not matter because
>> it is not tied to driver implementation or device programming model. The
>> driver maps ID to respective clock.
>>
>> Using some meaningful numbers as these IDs, means you tied bindings to
>> your implementation and any change in implementation requires change in
>> the bindings. This contradicts the idea of bindings.
>>
> 
> I totally agree. Bindings ID are abstract numbers.
> We do follow that. We even document it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n118
> 
> It is just a choice to not expose some IDs.
> It is not tied to the implementation at all.
> I think we actually follow the rules and the idea behind it.
> 
> We can expose then all If you still think what we are doing is not appropriate.

No, no need. You are right and I took your not-by-one-increment-ID by
other approaches I saw.

The IDs do not have to be incremental, they should not be tied to
programming model.

You have it done and documented, so thanks for explanation:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Krzysztof Kozlowski]

On 28/07/2022 12:05, Yu Tu wrote:
> Hi Krzysztof,
> 	Thanks for your reply.
> 
> On 2022/7/28 16:41, Krzysztof Kozlowski wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On 28/07/2022 07:42, Yu Tu wrote:
>>> Add new clock controller compatible and dt-bindings header for the
>>> Everything-Else domain of the S4 SoC.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>
>>
>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index c1abc53f9e91..f872d0c0c253 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>>>   F:	drivers/clk/meson/
>>>   F:	include/dt-bindings/clock/gxbb*
>>>   F:	include/dt-bindings/clock/meson*
>>> +F:	include/dt-bindings/clock/s4-clkc.h
>>>   
>>>   ARM/Amlogic Meson SoC Crypto Drivers
>>>   M:	Corentin Labbe <clabbe@baylibre.com>
>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
>>> new file mode 100644
>>> index 000000000000..b686c8877419
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/s4-clkc.h
>>
>> Filename with vendor prefix, so:
>> amlogic,s4-clkc.h
> It's fine with me. It's mainly Jerome's opinion.

To clarify: I understand such naming might bring inconsistency, but we
want to bring some order in the bindings directories. They keep growing
and at some point the model names might start conflicting.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Yu Tu]

On 2022/7/28 18:09, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
> 
> On 28/07/2022 12:05, Yu Tu wrote:
>> Hi Krzysztof,
>> 	Thanks for your reply.
>>
>> On 2022/7/28 16:41, Krzysztof Kozlowski wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On 28/07/2022 07:42, Yu Tu wrote:
>>>> Add new clock controller compatible and dt-bindings header for the
>>>> Everything-Else domain of the S4 SoC.
>>>>
>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>
>>>
>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index c1abc53f9e91..f872d0c0c253 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>>>>    F:	drivers/clk/meson/
>>>>    F:	include/dt-bindings/clock/gxbb*
>>>>    F:	include/dt-bindings/clock/meson*
>>>> +F:	include/dt-bindings/clock/s4-clkc.h
>>>>    
>>>>    ARM/Amlogic Meson SoC Crypto Drivers
>>>>    M:	Corentin Labbe <clabbe@baylibre.com>
>>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
>>>> new file mode 100644
>>>> index 000000000000..b686c8877419
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/clock/s4-clkc.h
>>>
>>> Filename with vendor prefix, so:
>>> amlogic,s4-clkc.h
>> It's fine with me. It's mainly Jerome's opinion.
> 
> To clarify: I understand such naming might bring inconsistency, but we
> want to bring some order in the bindings directories. They keep growing
> and at some point the model names might start conflicting.
If Jerome agrees, I will change it according to your opinion and make 
another edition.

> 
> 
> Best regards,
> Krzysztof
> 
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Jerome Brunet]

On Thu 28 Jul 2022 at 18:19, Yu Tu <yu.tu@amlogic.com> wrote:

> On 2022/7/28 18:09, Krzysztof Kozlowski wrote:
>> [ EXTERNAL EMAIL ]
>> On 28/07/2022 12:05, Yu Tu wrote:
>>> Hi Krzysztof,
>>> 	Thanks for your reply.
>>>
>>> On 2022/7/28 16:41, Krzysztof Kozlowski wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On 28/07/2022 07:42, Yu Tu wrote:
>>>>> Add new clock controller compatible and dt-bindings header for the
>>>>> Everything-Else domain of the S4 SoC.
>>>>>
>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>
>>>>
>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index c1abc53f9e91..f872d0c0c253 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>>>>>    F:	drivers/clk/meson/
>>>>>    F:	include/dt-bindings/clock/gxbb*
>>>>>    F:	include/dt-bindings/clock/meson*
>>>>> +F:	include/dt-bindings/clock/s4-clkc.h
>>>>>       ARM/Amlogic Meson SoC Crypto Drivers
>>>>>    M:	Corentin Labbe <clabbe@baylibre.com>
>>>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
>>>>> new file mode 100644
>>>>> index 000000000000..b686c8877419
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/clock/s4-clkc.h
>>>>
>>>> Filename with vendor prefix, so:
>>>> amlogic,s4-clkc.h
>>> It's fine with me. It's mainly Jerome's opinion.
>> To clarify: I understand such naming might bring inconsistency, but we
>> want to bring some order in the bindings directories. They keep growing
>> and at some point the model names might start conflicting.
> If Jerome agrees, I will change it according to your opinion and make
> another edition.

I'm aligned with Krzysztof on this. Please add the vendor prefix.

It was mistake to omit the vendor prefix. Unfortunately, I don't think
we can fix the old bindings now.

>
>> 
>> Best regards,
>> Krzysztof
>> .


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Yu Tu]

On 2022/7/28 19:48, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Thu 28 Jul 2022 at 18:19, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> On 2022/7/28 18:09, Krzysztof Kozlowski wrote:
>>> [ EXTERNAL EMAIL ]
>>> On 28/07/2022 12:05, Yu Tu wrote:
>>>> Hi Krzysztof,
>>>> 	Thanks for your reply.
>>>>
>>>> On 2022/7/28 16:41, Krzysztof Kozlowski wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> On 28/07/2022 07:42, Yu Tu wrote:
>>>>>> Add new clock controller compatible and dt-bindings header for the
>>>>>> Everything-Else domain of the S4 SoC.
>>>>>>
>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>>
>>>>>
>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index c1abc53f9e91..f872d0c0c253 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -1775,6 +1775,7 @@ F:	Documentation/devicetree/bindings/clock/amlogic*
>>>>>>     F:	drivers/clk/meson/
>>>>>>     F:	include/dt-bindings/clock/gxbb*
>>>>>>     F:	include/dt-bindings/clock/meson*
>>>>>> +F:	include/dt-bindings/clock/s4-clkc.h
>>>>>>        ARM/Amlogic Meson SoC Crypto Drivers
>>>>>>     M:	Corentin Labbe <clabbe@baylibre.com>
>>>>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h
>>>>>> new file mode 100644
>>>>>> index 000000000000..b686c8877419
>>>>>> --- /dev/null
>>>>>> +++ b/include/dt-bindings/clock/s4-clkc.h
>>>>>
>>>>> Filename with vendor prefix, so:
>>>>> amlogic,s4-clkc.h
>>>> It's fine with me. It's mainly Jerome's opinion.
>>> To clarify: I understand such naming might bring inconsistency, but we
>>> want to bring some order in the bindings directories. They keep growing
>>> and at some point the model names might start conflicting.
>> If Jerome agrees, I will change it according to your opinion and make
>> another edition.
> 
> I'm aligned with Krzysztof on this. Please add the vendor prefix.
I will add it in next version.

> 
> It was mistake to omit the vendor prefix. Unfortunately, I don't think
> we can fix the old bindings now.
> 
>>
>>>
>>> Best regards,
>>> Krzysztof
>>> .
> 
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel