diff mbox series

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

Message ID 20190118131639.17578-5-k.konieczny@partner.samsung.com (mailing list archive)
State Superseded, archived
Headers show
Series Add imem clock for Exynos 5433 | expand

Commit Message

Kamil Konieczny Jan. 18, 2019, 1:16 p.m. UTC
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(+)

Comments

Chanwoo Choi Jan. 19, 2019, 12:40 p.m. UTC | #1
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
>
Rob Herring (Arm) Jan. 21, 2019, 2:51 p.m. UTC | #2
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
Rob Herring (Arm) Jan. 21, 2019, 2:52 p.m. UTC | #3
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>
Chanwoo Choi Jan. 22, 2019, 12:27 a.m. UTC | #4
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
> 
>
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
diff mbox series

Patch

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 */