| Message ID | 20190118131639.17578-5-k.konieczny@partner.samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add imem clock for Exynos 5433 | expand |
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 >
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
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>
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 --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 */
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(+)