[PATCH v4 0/5] Add imem clock for Exynos 5433

[PATCH v4 0/5] Add imem clock for Exynos 5433

[Kamil Konieczny]

Add imem clock for Exynos 5433. This will allow to use SlimSSS (Slim
Security SubSystem) with crypto functions.

Changes since v3:
- added Acked-by and Reviewed-by to patches 2 and 3
- dropped "[PATCH v3 4/5] arm64: dts: exynos: add imem clock" because it
  was already applied
- split "[PATCH v3 5/5] clk: samsung: exynos5433: add imem clocks"
  into two parts, first for DT bindings and second for clock driver. In
  clock driver I dropped all SSS clocks and left only two slimSSS, as I
  was unable to test SSS with crypto driver s5p-sss.

Changes since v2:
- added Reviewed-by to two first patches
- fixed subject for 3rd patch and commit message
- in patch 3 moved documentation of imem to end of clocks after CMU_CAM1
- in patch 4 moved cmu_imem declarations after cmu_cam1
- in patch 5 added newlines after clock registers, moved code after cmu_cam1,
  removed CLK_OF_DECLARE and exynos5433_cmu_imem_init, moved cmu_imem
  compatible to the end of exynos5433_cmu_of_match

Changes since v1:
- splitted typo patch into two, one of them for stable, suggested by Krzysztof
  Kozlowski
- added more registers as suggested by Chanwoo Choi

Kamil Konieczny (5):
  clk: samsung: exynos5433: fix typo in imem divider
  clk: samsung: exynos5433: fix name typo in sssx
  dt-bindings: clk: exynos5433: document imem clock
  dt-bindings: clk: exynos5433: add imem clock
  clk: samsung: exynos5433: add imem clocks

 .../bindings/clock/exynos5433-clock.txt       | 23 ++++++++
 drivers/clk/samsung/clk-exynos5433.c          | 38 ++++++++++++-
 include/dt-bindings/clock/exynos5433.h        | 57 ++++++++++++++++++-
 3 files changed, 114 insertions(+), 4 deletions(-)

-- 
2.20.0

[PATCH v4 1/5] clk: samsung: exynos5433: fix typo in imem divider

[Kamil Konieczny]

Fix typo in imem clock divider 200 switched with 266.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 drivers/clk/samsung/clk-exynos5433.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 751e2c4fb65b..ea47f49abc7f 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -568,10 +568,10 @@ static const struct samsung_gate_clock top_gate_clks[] __initconst = {
 	GATE(CLK_ACLK_BUS1_400, "aclk_bus1_400", "div_aclk_bus1_400",
 			ENABLE_ACLK_TOP, 25,
 			CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0),
-	GATE(CLK_ACLK_IMEM_200, "aclk_imem_200", "div_aclk_imem_266",
+	GATE(CLK_ACLK_IMEM_200, "aclk_imem_200", "div_aclk_imem_200",
 			ENABLE_ACLK_TOP, 24,
 			CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0),
-	GATE(CLK_ACLK_IMEM_266, "aclk_imem_266", "div_aclk_imem_200",
+	GATE(CLK_ACLK_IMEM_266, "aclk_imem_266", "div_aclk_imem_266",
 			ENABLE_ACLK_TOP, 23,
 			CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_ACLK_PERIC_66, "aclk_peric_66", "div_aclk_peric_66_b",
-- 
2.20.0

[PATCH v4 2/5] clk: samsung: exynos5433: fix name typo in sssx

[Kamil Konieczny]

Fix typo in sssx name, there should be three letters 's'.

Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 drivers/clk/samsung/clk-exynos5433.c   | 2 +-
 include/dt-bindings/clock/exynos5433.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index ea47f49abc7f..24c3360db65b 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -559,7 +559,7 @@ static const struct samsung_gate_clock top_gate_clks[] __initconst = {
 	/* ENABLE_ACLK_TOP */
 	GATE(CLK_ACLK_G3D_400, "aclk_g3d_400", "div_aclk_g3d_400",
 			ENABLE_ACLK_TOP, 30, CLK_IS_CRITICAL, 0),
-	GATE(CLK_ACLK_IMEM_SSX_266, "aclk_imem_ssx_266",
+	GATE(CLK_ACLK_IMEM_SSSX_266, "aclk_imem_sssx_266",
 			"div_aclk_imem_sssx_266", ENABLE_ACLK_TOP,
 			29, CLK_IGNORE_UNUSED, 0),
 	GATE(CLK_ACLK_BUS0_400, "aclk_bus0_400", "div_aclk_bus0_400",
diff --git a/include/dt-bindings/clock/exynos5433.h b/include/dt-bindings/clock/exynos5433.h
index 98bd85ce1e45..87bb2b017143 100644
--- a/include/dt-bindings/clock/exynos5433.h
+++ b/include/dt-bindings/clock/exynos5433.h
@@ -156,7 +156,7 @@
 #define CLK_ACLK_G2D_266		220
 #define CLK_ACLK_G2D_400		221
 #define CLK_ACLK_G3D_400		222
-#define CLK_ACLK_IMEM_SSX_266		223
+#define CLK_ACLK_IMEM_SSSX_266		223
 #define CLK_ACLK_BUS0_400		224
 #define CLK_ACLK_BUS1_400		225
 #define CLK_ACLK_IMEM_200		226
-- 
2.20.0

[PATCH v4 3/5] dt-bindings: clk: exynos5433: document imem clock

[Kamil Konieczny]

Document DT bindings for imem clock of the Samsung Exynos5433 SSS (Security
SubSystem) and SlimSSS IPs.

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 .../bindings/clock/exynos5433-clock.txt       | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/exynos5433-clock.txt b/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
index 50d5897c9849..183c327a7d6b 100644
--- a/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
+++ b/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
@@ -50,6 +50,8 @@ Required Properties:
     IPs.
   - "samsung,exynos5433-cmu-cam1" - clock controller compatible for CMU_CAM1
     which generates clocks for Cortex-A5/MIPI_CSIS2/FIMC-LITE_C/FIMC-FD IPs.
+  - "samsung,exynos5433-cmu-imem"   - clock controller compatible for CMU_IMEM
+    which generates clocks for SSS (Security SubSystem) and SlimSSS IPs.
 
 - reg: physical base address of the controller and length of memory mapped
   region.
@@ -168,6 +170,12 @@ Required Properties:
 		- aclk_cam1_400
 		- aclk_cam1_552
 
+	Input clocks for imem clock controller:
+		- oscclk
+		- aclk_imem_sssx_266
+		- aclk_imem_266
+		- aclk_imem_200
+
 Optional properties:
   - power-domains: a phandle to respective power domain node as described by
 	generic PM domain bindings (see power/power_domain.txt for more
@@ -469,6 +477,21 @@ Example 2: Examples of clock controller nodes are listed below.
 		power-domains = <&pd_cam1>;
 	};
 
+	cmu_imem: clock-controller@11060000 {
+		compatible = "samsung,exynos5433-cmu-imem";
+		reg = <0x11060000 0x1000>;
+		#clock-cells = <1>;
+
+		clock-names = "oscclk",
+			"aclk_imem_sssx_266",
+			"aclk_imem_266",
+			"aclk_imem_200";
+		clocks = <&xxti>,
+			<&cmu_top CLK_DIV_ACLK_IMEM_SSSX_266>,
+			<&cmu_top CLK_DIV_ACLK_IMEM_266>,
+			<&cmu_top CLK_DIV_ACLK_IMEM_200>;
+	};
+
 Example 3: UART controller node that consumes the clock generated by the clock
 	   controller.
 
-- 
2.20.0

[PATCH v4 4/5] dt-bindings: clk: exynos5433: add imem clock

[Kamil Konieczny]

Add DT bindings to describe imem clocks for SSS (Security SubSystem) and
SlimSSS in Exynos5433.

Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 include/dt-bindings/clock/exynos5433.h | 55 ++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/include/dt-bindings/clock/exynos5433.h b/include/dt-bindings/clock/exynos5433.h
index 87bb2b017143..cc6153a0b5f7 100644
--- a/include/dt-bindings/clock/exynos5433.h
+++ b/include/dt-bindings/clock/exynos5433.h
@@ -1406,4 +1406,59 @@
 
 #define CAM1_NR_CLK					113
 
+/* CMU_IMEM */
+#define CLK_ACLK_SSS			1
+#define CLK_ACLK_SLIMSSS		2
+#define CLK_ACLK_RTIC			3
+#define CLK_ACLK_XIU_SSSX		4
+#define CLK_ACLK_ASYNCAHBM_SSS_ATLAS	5
+#define CLK_ACLK_ASYNCAXI_SSSX		6
+#define CLK_ACLK_BTS_SSS_CCI		7
+#define CLK_ACLK_BTS_SSS_DRAM		8
+#define CLK_ACLK_BTS_SLIMSSS		9
+#define CLK_ACLK_SMMU_SSS_CCI		10
+#define CLK_ACLK_SMMU_SSS_DRAM		11
+#define CLK_ACLK_SMMU_SLIMSSS		12
+#define CLK_ACLK_SMMU_RTIC		13
+#define CLK_ACLK_IMEMND_266		14
+#define CLK_ACLK_ALB_IMEM		15
+#define CLK_ACLK_XIU_IMEMX		16
+#define CLK_ACLK_AXIUS_IMEMX		17
+#define CLK_ACLK_ASYNCAXI_IMEMX		18
+#define CLK_ACLK_ARBG_TX		19
+#define CLK_ACLK_BTS_ARBG_TX		20
+#define CLK_ACLK_SMMU_ARBG_TX		21
+#define CLK_ACLK_GIC			22
+#define CLK_ACLK_INT_MEM		23
+#define CLK_ACLK_XIU_PIMEMX		24
+#define CLK_ACLK_AXI2APB_IMEM0P		25
+#define CLK_ACLK_AXI2APB_IMEM1P		26
+#define CLK_ACLK_ASYNCAXIS_MIF_PIMEMX	27
+#define CLK_ACLK_AXIDS_PIMEMX_GIC	28
+#define CLK_ACLK_AXIDS_PIMEMX_IMEM0P	29
+#define CLK_ACLK_AXIDS_PIMEMX_IMEM1P	30
+#define CLK_ACLK_SROMC			31
+#define CLK_ACLK_AXIDS_SROMC		32
+#define CLK_ACLK_AXI2AHB_IMEMH		33
+#define CLK_PCLK_SSS			34
+#define CLK_PCLK_SLIMSSS		35
+#define CLK_PCLK_RTIC			36
+#define CLK_PCLK_SYSREG_IMEM		37
+#define CLK_PCLK_PMU_IMEM		38
+#define CLK_PCLK_ALB_IMEM		39
+#define CLK_PCLK_BTS_SSS_CCI		40
+#define CLK_PCLK_BTS_SSS_DRAM		41
+#define CLK_PCLK_BTS_SLIMSSS		42
+#define CLK_PCLK_SMMU_SSS_CCI		43
+#define CLK_PCLK_SMMU_SSS_DRAM		44
+#define CLK_PCLK_SMMU_SLIMSSS		45
+#define CLK_PCLK_SMMU_RTIC		46
+#define CLK_PCLK_ASYNCAPB_ARBG_TX	47
+#define CLK_PCLK_BTS_ARBG_TX		48
+#define CLK_PCLK_SMMU_ARBG_TX		49
+#define CLK_PCLK_ASYNCAXI_IMEMX		50
+#define CLK_PCLK_GPIO_IMEM		51
+
+#define IMEM_NR_CLK			52
+
 #endif /* _DT_BINDINGS_CLOCK_EXYNOS5433_H */
-- 
2.20.0

Re: [PATCH v4 4/5] dt-bindings: clk: exynos5433: add imem clock

[Chanwoo Choi]

Hi Kamil,

This version only adds following two clocks without that other clocks
be included in imem block.
- CLK_ACLK_SLIMSSS
- CLK_PCLK_SLIMSSS

Usually, the users only refer to
'include/dt-bindings/clock/exynos5433.h' in order to get the supported
clocks. It means that if there are different between the clock list in
'include/dt-bindings/clock/exynos5433.h' and the supported clock list
of drivers/clock/', it make the confusion for users. So,
'include/dt-bindings/clock/exynos5433.h' have to define the only
supported clock ids.

Please remove other clock ids except for CLK_ACLK_SLIMSSS/CLK_PCLK_SLIMSSS.

Best Regards,
Chanwoo Choi
Samsung Electronics

2019년 1월 18일 (금) 오후 10:17, Kamil Konieczny
<k.konieczny@partner.samsung.com>님이 작성:
>
> Add DT bindings to describe imem clocks for SSS (Security SubSystem) and
> SlimSSS in Exynos5433.
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  include/dt-bindings/clock/exynos5433.h | 55 ++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/include/dt-bindings/clock/exynos5433.h b/include/dt-bindings/clock/exynos5433.h
> index 87bb2b017143..cc6153a0b5f7 100644
> --- a/include/dt-bindings/clock/exynos5433.h
> +++ b/include/dt-bindings/clock/exynos5433.h
> @@ -1406,4 +1406,59 @@
>
>  #define CAM1_NR_CLK                                    113
>
> +/* CMU_IMEM */
> +#define CLK_ACLK_SSS                   1
> +#define CLK_ACLK_SLIMSSS               2
> +#define CLK_ACLK_RTIC                  3
> +#define CLK_ACLK_XIU_SSSX              4
> +#define CLK_ACLK_ASYNCAHBM_SSS_ATLAS   5
> +#define CLK_ACLK_ASYNCAXI_SSSX         6
> +#define CLK_ACLK_BTS_SSS_CCI           7
> +#define CLK_ACLK_BTS_SSS_DRAM          8
> +#define CLK_ACLK_BTS_SLIMSSS           9
> +#define CLK_ACLK_SMMU_SSS_CCI          10
> +#define CLK_ACLK_SMMU_SSS_DRAM         11
> +#define CLK_ACLK_SMMU_SLIMSSS          12
> +#define CLK_ACLK_SMMU_RTIC             13
> +#define CLK_ACLK_IMEMND_266            14
> +#define CLK_ACLK_ALB_IMEM              15
> +#define CLK_ACLK_XIU_IMEMX             16
> +#define CLK_ACLK_AXIUS_IMEMX           17
> +#define CLK_ACLK_ASYNCAXI_IMEMX                18
> +#define CLK_ACLK_ARBG_TX               19
> +#define CLK_ACLK_BTS_ARBG_TX           20
> +#define CLK_ACLK_SMMU_ARBG_TX          21
> +#define CLK_ACLK_GIC                   22
> +#define CLK_ACLK_INT_MEM               23
> +#define CLK_ACLK_XIU_PIMEMX            24
> +#define CLK_ACLK_AXI2APB_IMEM0P                25
> +#define CLK_ACLK_AXI2APB_IMEM1P                26
> +#define CLK_ACLK_ASYNCAXIS_MIF_PIMEMX  27
> +#define CLK_ACLK_AXIDS_PIMEMX_GIC      28
> +#define CLK_ACLK_AXIDS_PIMEMX_IMEM0P   29
> +#define CLK_ACLK_AXIDS_PIMEMX_IMEM1P   30
> +#define CLK_ACLK_SROMC                 31
> +#define CLK_ACLK_AXIDS_SROMC           32
> +#define CLK_ACLK_AXI2AHB_IMEMH         33
> +#define CLK_PCLK_SSS                   34
> +#define CLK_PCLK_SLIMSSS               35
> +#define CLK_PCLK_RTIC                  36
> +#define CLK_PCLK_SYSREG_IMEM           37
> +#define CLK_PCLK_PMU_IMEM              38
> +#define CLK_PCLK_ALB_IMEM              39
> +#define CLK_PCLK_BTS_SSS_CCI           40
> +#define CLK_PCLK_BTS_SSS_DRAM          41
> +#define CLK_PCLK_BTS_SLIMSSS           42
> +#define CLK_PCLK_SMMU_SSS_CCI          43
> +#define CLK_PCLK_SMMU_SSS_DRAM         44
> +#define CLK_PCLK_SMMU_SLIMSSS          45
> +#define CLK_PCLK_SMMU_RTIC             46
> +#define CLK_PCLK_ASYNCAPB_ARBG_TX      47
> +#define CLK_PCLK_BTS_ARBG_TX           48
> +#define CLK_PCLK_SMMU_ARBG_TX          49
> +#define CLK_PCLK_ASYNCAXI_IMEMX                50
> +#define CLK_PCLK_GPIO_IMEM             51
> +
> +#define IMEM_NR_CLK                    52
> +
>  #endif /* _DT_BINDINGS_CLOCK_EXYNOS5433_H */
> --
> 2.20.0
>

Re: [PATCH v4 4/5] dt-bindings: clk: exynos5433: add imem clock

[Rob Herring]

On Sat, Jan 19, 2019 at 09:40:10PM +0900, Chanwoo Choi wrote:
> Hi Kamil,
> 
> This version only adds following two clocks without that other clocks
> be included in imem block.
> - CLK_ACLK_SLIMSSS
> - CLK_PCLK_SLIMSSS
> 
> Usually, the users only refer to
> 'include/dt-bindings/clock/exynos5433.h' in order to get the supported
> clocks. It means that if there are different between the clock list in
> 'include/dt-bindings/clock/exynos5433.h' and the supported clock list
> of drivers/clock/', it make the confusion for users. So,
> 'include/dt-bindings/clock/exynos5433.h' have to define the only
> supported clock ids.
> 
> Please remove other clock ids except for CLK_ACLK_SLIMSSS/CLK_PCLK_SLIMSSS.

And add clocks 1-by-1 instead? The binding headers are a pain to merge 
because they are a dts and driver dependency. Having a complete header 
is preferred unless there is doubt in the correctness of it.

The driver can warn users about unsupported IDs.

Rob

Re: [PATCH v4 4/5] dt-bindings: clk: exynos5433: add imem clock

[Rob Herring]

On Fri, 18 Jan 2019 14:16:38 +0100, Kamil Konieczny wrote:
> Add DT bindings to describe imem clocks for SSS (Security SubSystem) and
> SlimSSS in Exynos5433.
> 
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  include/dt-bindings/clock/exynos5433.h | 55 ++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 4/5] dt-bindings: clk: exynos5433: add imem clock

[Chanwoo Choi]

On 19. 1. 21. 오후 11:51, Rob Herring wrote:
> On Sat, Jan 19, 2019 at 09:40:10PM +0900, Chanwoo Choi wrote:
>> Hi Kamil,
>>
>> This version only adds following two clocks without that other clocks
>> be included in imem block.
>> - CLK_ACLK_SLIMSSS
>> - CLK_PCLK_SLIMSSS
>>
>> Usually, the users only refer to
>> 'include/dt-bindings/clock/exynos5433.h' in order to get the supported
>> clocks. It means that if there are different between the clock list in
>> 'include/dt-bindings/clock/exynos5433.h' and the supported clock list
>> of drivers/clock/', it make the confusion for users. So,
>> 'include/dt-bindings/clock/exynos5433.h' have to define the only
>> supported clock ids.
>>
>> Please remove other clock ids except for CLK_ACLK_SLIMSSS/CLK_PCLK_SLIMSSS.
> 
> And add clocks 1-by-1 instead? The binding headers are a pain to merge 
> because they are a dts and driver dependency. Having a complete header 
> is preferred unless there is doubt in the correctness of it.

As you commented, I knew the dependency between dt-binding head file and
drivers/dts. But, IMO, I'm not sure that it is right to define
the unsupported ID in dt-bindings header file because we don't know
the correctness without any real test. The author explained why dropped
the clocks except for two clocks on cover-letter. It means that the clocks
except for two clocks might be never added to clock driver because He was
unable to test them..

> 
> The driver can warn users about unsupported IDs.
> 
> Rob
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v4 4/5] dt-bindings: clk: exynos5433: add imem clock

[Sylwester Nawrocki]

On 1/22/19 01:27, Chanwoo Choi wrote:
> On 19. 1. 21. 오후 11:51, Rob Herring wrote:
>> On Sat, Jan 19, 2019 at 09:40:10PM +0900, Chanwoo Choi wrote:

>>> This version only adds following two clocks without that other clocks
>>> be included in imem block.
>>> - CLK_ACLK_SLIMSSS
>>> - CLK_PCLK_SLIMSSS
>>>
>>> Usually, the users only refer to
>>> 'include/dt-bindings/clock/exynos5433.h' in order to get the supported
>>> clocks. It means that if there are different between the clock list in
>>> 'include/dt-bindings/clock/exynos5433.h' and the supported clock list
>>> of drivers/clock/', it make the confusion for users. So,
>>> 'include/dt-bindings/clock/exynos5433.h' have to define the only
>>> supported clock ids.
>>>
>>> Please remove other clock ids except for CLK_ACLK_SLIMSSS/CLK_PCLK_SLIMSSS.
>>
>> And add clocks 1-by-1 instead? The binding headers are a pain to merge 
>> because they are a dts and driver dependency. Having a complete header 
>> is preferred unless there is doubt in the correctness of it.

Exactly, this is also what we agreed with Stephen, to make a complete header
with all IDs. I believe the list of clocks is correct, this clock unit is
pretty straightforward, it's just a collection of gate clocks.
But chances are that the other clocks will never get added, as those seem
to be normally handled outside of Linux kernel. It still might not be a good
argument against having complete header describing the hardware completely,
independently of an OS, the bootloader might use those IDs, etc.
In practise it will be adding likely never used code though.

> As you commented, I knew the dependency between dt-binding head file and
> drivers/dts. But, IMO, I'm not sure that it is right to define
> the unsupported ID in dt-bindings header file because we don't know
> the correctness without any real test. The author explained why dropped
> the clocks except for two clocks on cover-letter. It means that the clocks
> except for two clocks might be never added to clock driver because He was
> unable to test them..

I agree, I doubt the remaining clocks will ever get added in case of this SoC. 

>> The driver can warn users about unsupported IDs
-- 
Regards,
Sylwester