diff mbox

[11/19] clk: samsung: exynos5433: Add clocks for CMU_BUS{0|1|2} domains

Message ID 1417073716-22997-12-git-send-email-cw00.choi@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chanwoo Choi Nov. 27, 2014, 7:35 a.m. UTC
This patch adds the mux/divider/gate clocks for CMU_BUS{0|1|2} domains
which contain global data buses clocked at up the 400MHz. These blocks
transfer data between DRAM and various sub-blocks. These clock domains
also contain global peripheral buses clocked at 67/111/200/222/266/333/400
MHz and used for regiser accesses.

Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Geunsik Lim <geunsik.lim@samsung.com>
---
 .../devicetree/bindings/clock/exynos5433-clock.txt |  21 ++
 drivers/clk/samsung/clk-exynos5433.c               | 225 ++++++++++++++++++++-
 include/dt-bindings/clock/exynos5433.h             |  52 ++++-
 3 files changed, 295 insertions(+), 3 deletions(-)

Comments

Arnd Bergmann Nov. 27, 2014, 11:41 a.m. UTC | #1
On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
> +    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
> +    which generates global data buses clock and global peripheral buses clock.
>  
>  - reg: physical base address of the controller and length of memory mapped
>    region.
> 

This looks like you are duplicating the bindings and the code, but
it's really the same hardware multiple times with minor variations
that you should be able to describe properly here. Why not make
three nodes with the same compatible string and have them handled
by the same code?

	Arnd
Chanwoo Choi Nov. 27, 2014, 11:56 a.m. UTC | #2
Dear Arnd,

On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
> On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
>> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
>> +    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
>> +    which generates global data buses clock and global peripheral buses clock.
>>  
>>  - reg: physical base address of the controller and length of memory mapped
>>    region.
>>
> 
> This looks like you are duplicating the bindings and the code, but
> it's really the same hardware multiple times with minor variations
> that you should be able to describe properly here. Why not make
> three nodes with the same compatible string and have them handled
> by the same code?

Each CMU_BUSx domain of Exynos5433 have different base address as following:
- CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
- CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
- CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04

So, I implement CMU_BUSx domain which has each compatible string.

Best Regards,
Chanwoo Choi
Hi,

On 27/11/14 12:56, Chanwoo Choi wrote:
> On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
>> > On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
>>> >> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
>>> >> +    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
>>> >> +    which generates global data buses clock and global peripheral buses clock.
>>> >>  
>>> >>  - reg: physical base address of the controller and length of memory mapped
>>> >>    region.
>>> >>
>> > 
>> > This looks like you are duplicating the bindings and the code, but
>> > it's really the same hardware multiple times with minor variations
>> > that you should be able to describe properly here. Why not make
>> > three nodes with the same compatible string and have them handled
>> > by the same code?
>
> Each CMU_BUSx domain of Exynos5433 have different base address as following:
> - CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
> - CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
> - CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04
> 
> So, I implement CMU_BUSx domain which has each compatible string.

You can always have multiple entries in the reg property. I've done
something like this for the exynos4415 CMU_ISPx units:

	cmu_isp: clock-controller@12060000 {
		compatible = "samsung,exynos4415-cmu-isp";
		reg = <0x12060000 0xB10>, <0x12070000 0xB10>;
		#clock-cells = <1>;

		assigned-clocks = <&cmu CLK_FOUT_ISP_PLL>;
		assigned-clock-rates = <300000000>;
	};

--
Regards,
Sylwester
Chanwoo Choi Nov. 27, 2014, 12:14 p.m. UTC | #4
Hi Sylwester,

On 11/27/2014 09:12 PM, Sylwester Nawrocki wrote:
> Hi,
> 
> On 27/11/14 12:56, Chanwoo Choi wrote:
>> On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
>>>> On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
>>>>>> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
>>>>>> +    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
>>>>>> +    which generates global data buses clock and global peripheral buses clock.
>>>>>>  
>>>>>>  - reg: physical base address of the controller and length of memory mapped
>>>>>>    region.
>>>>>>
>>>>
>>>> This looks like you are duplicating the bindings and the code, but
>>>> it's really the same hardware multiple times with minor variations
>>>> that you should be able to describe properly here. Why not make
>>>> three nodes with the same compatible string and have them handled
>>>> by the same code?
>>
>> Each CMU_BUSx domain of Exynos5433 have different base address as following:
>> - CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
>> - CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
>> - CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04
>>
>> So, I implement CMU_BUSx domain which has each compatible string.
> 
> You can always have multiple entries in the reg property. I've done
> something like this for the exynos4415 CMU_ISPx units:
> 
> 	cmu_isp: clock-controller@12060000 {
> 		compatible = "samsung,exynos4415-cmu-isp";
> 		reg = <0x12060000 0xB10>, <0x12070000 0xB10>;
> 		#clock-cells = <1>;
> 
> 		assigned-clocks = <&cmu CLK_FOUT_ISP_PLL>;
> 		assigned-clock-rates = <300000000>;
> 	};

Thanks for your guide.

I'll re-implment CMU_BUSx domain according to your guide.

Best Regards,
Chanwoo Choi
Arnd Bergmann Nov. 27, 2014, 12:35 p.m. UTC | #5
On Thursday 27 November 2014 13:12:08 Sylwester Nawrocki wrote:
> On 27/11/14 12:56, Chanwoo Choi wrote:
> > On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
> >> > On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
> >>> >> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
> >>> >> +    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
> >>> >> +    which generates global data buses clock and global peripheral buses clock.
> >>> >>  
> >>> >>  - reg: physical base address of the controller and length of memory mapped
> >>> >>    region.
> >>> >>
> >> > 
> >> > This looks like you are duplicating the bindings and the code, but
> >> > it's really the same hardware multiple times with minor variations
> >> > that you should be able to describe properly here. Why not make
> >> > three nodes with the same compatible string and have them handled
> >> > by the same code?
> >
> > Each CMU_BUSx domain of Exynos5433 have different base address as following:
> > - CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
> > - CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
> > - CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04
> > 
> > So, I implement CMU_BUSx domain which has each compatible string.

But the base address is in the reg property, not in the compatible
property. What I mean is to have multiple nodes like

	clock-controller@113600000 {
		reg = <0 0x113600000 0 0x1000>;
		compatible = "samsung,exynos5433-cmu";
		#clock-cells = <1>;
	};

	clock-controller@114800000 {
		reg = <0 0x114800000 0 0x1000>;
		compatible = "samsung,exynos5433-cmu";
		#clock-cells = <1>;
	};

The code will just map the local registers for each instance and then
provide the clocks of the right instance when asked for it.

> You can always have multiple entries in the reg property. I've done
> something like this for the exynos4415 CMU_ISPx units:
> 
>         cmu_isp: clock-controller@12060000 {
>                 compatible = "samsung,exynos4415-cmu-isp";
>                 reg = <0x12060000 0xB10>, <0x12070000 0xB10>;
>                 #clock-cells = <1>;
> 
>                 assigned-clocks = <&cmu CLK_FOUT_ISP_PLL>;
>                 assigned-clock-rates = <300000000>;
>         };

This is a different problem, this is a clock controller with multiple
sets of registers that are all different. In case of the cmu, it seems
that they are all the same, you just have multiple copies at different
locations, and they are connected to different devices.

	Arnd
Chanwoo Choi Nov. 27, 2014, 12:58 p.m. UTC | #6
Dear Arnd,

On 11/27/2014 09:35 PM, Arnd Bergmann wrote:
> On Thursday 27 November 2014 13:12:08 Sylwester Nawrocki wrote:
>> On 27/11/14 12:56, Chanwoo Choi wrote:
>>> On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
>>>>> On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
>>>>>>> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
>>>>>>> +    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
>>>>>>> +    which generates global data buses clock and global peripheral buses clock.
>>>>>>>  
>>>>>>>  - reg: physical base address of the controller and length of memory mapped
>>>>>>>    region.
>>>>>>>
>>>>>
>>>>> This looks like you are duplicating the bindings and the code, but
>>>>> it's really the same hardware multiple times with minor variations
>>>>> that you should be able to describe properly here. Why not make
>>>>> three nodes with the same compatible string and have them handled
>>>>> by the same code?
>>>
>>> Each CMU_BUSx domain of Exynos5433 have different base address as following:
>>> - CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
>>> - CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
>>> - CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04
>>>
>>> So, I implement CMU_BUSx domain which has each compatible string.
> 
> But the base address is in the reg property, not in the compatible
> property. What I mean is to have multiple nodes like

The merged clock driver in mainline have different compatible string
if base addresss of clock domain is different. So, I implemented each CMU_BUSx domain
with different compatible string.

> 
> 	clock-controller@113600000 {
> 		reg = <0 0x113600000 0 0x1000>;
> 		compatible = "samsung,exynos5433-cmu";
> 		#clock-cells = <1>;
> 	};
> 
> 	clock-controller@114800000 {
> 		reg = <0 0x114800000 0 0x1000>;
> 		compatible = "samsung,exynos5433-cmu";
> 		#clock-cells = <1>;
> 	};
> 
> The code will just map the local registers for each instance and then
> provide the clocks of the right instance when asked for it.

Each clock domain has not the same mux/divider/clock. So, just one compatible
string could not support all of clock domains.

Best Regards,
Chanwoo Choi

> 
>> You can always have multiple entries in the reg property. I've done
>> something like this for the exynos4415 CMU_ISPx units:
>>
>>         cmu_isp: clock-controller@12060000 {
>>                 compatible = "samsung,exynos4415-cmu-isp";
>>                 reg = <0x12060000 0xB10>, <0x12070000 0xB10>;
>>                 #clock-cells = <1>;
>>
>>                 assigned-clocks = <&cmu CLK_FOUT_ISP_PLL>;
>>                 assigned-clock-rates = <300000000>;
>>         };
> 
> This is a different problem, this is a clock controller with multiple
> sets of registers that are all different. In case of the cmu, it seems
> that they are all the same, you just have multiple copies at different
> locations, and they are connected to different devices.
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Arnd Bergmann Nov. 27, 2014, 1:15 p.m. UTC | #7
On Thursday 27 November 2014 21:58:53 Chanwoo Choi wrote:
> Dear Arnd,
> 
> On 11/27/2014 09:35 PM, Arnd Bergmann wrote:
> > On Thursday 27 November 2014 13:12:08 Sylwester Nawrocki wrote:
> >> On 27/11/14 12:56, Chanwoo Choi wrote:
> >>> On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
> >>>>> On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
> >>>>>>> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
> >>>>>>> +    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
> >>>>>>> +    which generates global data buses clock and global peripheral buses clock.
> >>>>>>>  
> >>>>>>>  - reg: physical base address of the controller and length of memory mapped
> >>>>>>>    region.
> >>>>>>>
> >>>>>
> >>>>> This looks like you are duplicating the bindings and the code, but
> >>>>> it's really the same hardware multiple times with minor variations
> >>>>> that you should be able to describe properly here. Why not make
> >>>>> three nodes with the same compatible string and have them handled
> >>>>> by the same code?
> >>>
> >>> Each CMU_BUSx domain of Exynos5433 have different base address as following:
> >>> - CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
> >>> - CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
> >>> - CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04
> >>>
> >>> So, I implement CMU_BUSx domain which has each compatible string.
> > 
> > But the base address is in the reg property, not in the compatible
> > property. What I mean is to have multiple nodes like
> 
> The merged clock driver in mainline have different compatible string
> if base addresss of clock domain is different. So, I implemented each CMU_BUSx domain
> with different compatible string.

Why?

> > 	clock-controller@113600000 {
> > 		reg = <0 0x113600000 0 0x1000>;
> > 		compatible = "samsung,exynos5433-cmu";
> > 		#clock-cells = <1>;
> > 	};
> > 
> > 	clock-controller@114800000 {
> > 		reg = <0 0x114800000 0 0x1000>;
> > 		compatible = "samsung,exynos5433-cmu";
> > 		#clock-cells = <1>;
> > 	};
> > 
> > The code will just map the local registers for each instance and then
> > provide the clocks of the right instance when asked for it.
> 
> Each clock domain has not the same mux/divider/clock. So, just one compatible
> string could not support all of clock domains.

What are the specific differences? I saw that one of them has more
outputs than the others but it seemed like a superset, so you just
wouldn't be allowed to access the non-connected outputs.

	Arnd
Arnd Bergmann Nov. 27, 2014, 2:02 p.m. UTC | #8
On Thursday 27 November 2014 22:41:49 Chanwoo Choi wrote:
> 2014? 11? 27? ???, Arnd Bergmann<arnd@arndb.de>?? ??? ???:
> 
> > On Thursday 27 November 2014 21:58:53 Chanwoo Choi wrote:
> > > Dear Arnd,
> > >
> > > On 11/27/2014 09:35 PM, Arnd Bergmann wrote:
> > > > On Thursday 27 November 2014 13:12:08 Sylwester Nawrocki wrote:
> > > >> On 27/11/14 12:56, Chanwoo Choi wrote:
> > > >>> On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
> > > >>>>> On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
> > > >>>>>>> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
> > > >>>>>>> +    and "samsung,exynos5433-cmu-bus2" - clock controller
> > compatible for CMU_BUS
> > > >>>>>>> +    which generates global data buses clock and global
> > peripheral buses clock.
> > > >>>>>>>
> > > >>>>>>>  - reg: physical base address of the controller and length of
> > memory mapped
> > > >>>>>>>    region.
> > > >>>>>>>
> > > >>>>>
> > > >>>>> This looks like you are duplicating the bindings and the code, but
> > > >>>>> it's really the same hardware multiple times with minor variations
> > > >>>>> that you should be able to describe properly here. Why not make
> > > >>>>> three nodes with the same compatible string and have them handled
> > > >>>>> by the same code?
> > > >>>
> > > >>> Each CMU_BUSx domain of Exynos5433 have different base address as
> > following:
> > > >>> - CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
> > > >>> - CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
> > > >>> - CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04
> > > >>>
> > > >>> So, I implement CMU_BUSx domain which has each compatible string.
> > > >
> > > > But the base address is in the reg property, not in the compatible
> > > > property. What I mean is to have multiple nodes like
> > >
> > > The merged clock driver in mainline have different compatible string
> > > if base addresss of clock domain is different. So, I implemented each
> > CMU_BUSx domain
> > > with different compatible string.
> >
> > Why?
> 
> 
> As I explained on below, each clock domain have different clocks.
> So, clocks have unique clock name.
> 
>  If clock driver use only one compatible for various clock domain, clock
> driver have to know the base address of each domain for distinction of
> clock domain. I think It is stong dependency between device and driver.

No, not at all. You can have lots of clock controllers with the same
compatible string defining different instances of the same IP block,
e.g. for compatible="fixed-clock".

> >
> > > >     clock-controller@113600000 {
> > > >             reg = <0 0x113600000 0 0x1000>;
> > > >             compatible = "samsung,exynos5433-cmu";
> > > >             #clock-cells = <1>;
> > > >     };
> > > >
> > > >     clock-controller@114800000 {
> > > >             reg = <0 0x114800000 0 0x1000>;
> > > >             compatible = "samsung,exynos5433-cmu";
> > > >             #clock-cells = <1>;
> > > >     };
> > > >
> > > > The code will just map the local registers for each instance and then
> > > > provide the clocks of the right instance when asked for it.
> > >
> > > Each clock domain has not the same mux/divider/clock. So, just one
> > compatible
> > > string could not support all of clock domains.
> >
> > What are the specific differences?
> 
> 
> 
> > I'm not sure that difference among clock domains because I think it is
> dependent on the opinion of architect of SoC.
> 
> cmu_bus0/1/2 are much similar. Just cmu_bus2 has one more mux/gate clock
> than cmu_bus0/1.

Yes, that's what I mean. You can simply model that extra mux/gate
in the driver, as long as nothing ever tries to access the clock.

	Arnd
Chanwoo Choi Nov. 27, 2014, 3:17 p.m. UTC | #9
On Thu, Nov 27, 2014 at 11:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 27 November 2014 22:41:49 Chanwoo Choi wrote:
>> 2014? 11? 27? ???, Arnd Bergmann<arnd@arndb.de>?? ??? ???:
>>
>> > On Thursday 27 November 2014 21:58:53 Chanwoo Choi wrote:
>> > > Dear Arnd,
>> > >
>> > > On 11/27/2014 09:35 PM, Arnd Bergmann wrote:
>> > > > On Thursday 27 November 2014 13:12:08 Sylwester Nawrocki wrote:
>> > > >> On 27/11/14 12:56, Chanwoo Choi wrote:
>> > > >>> On 11/27/2014 08:41 PM, Arnd Bergmann wrote:
>> > > >>>>> On Thursday 27 November 2014 16:35:08 Chanwoo Choi wrote:
>> > > >>>>>>> +  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
>> > > >>>>>>> +    and "samsung,exynos5433-cmu-bus2" - clock controller
>> > compatible for CMU_BUS
>> > > >>>>>>> +    which generates global data buses clock and global
>> > peripheral buses clock.
>> > > >>>>>>>
>> > > >>>>>>>  - reg: physical base address of the controller and length of
>> > memory mapped
>> > > >>>>>>>    region.
>> > > >>>>>>>
>> > > >>>>>
>> > > >>>>> This looks like you are duplicating the bindings and the code, but
>> > > >>>>> it's really the same hardware multiple times with minor variations
>> > > >>>>> that you should be able to describe properly here. Why not make
>> > > >>>>> three nodes with the same compatible string and have them handled
>> > > >>>>> by the same code?
>> > > >>>
>> > > >>> Each CMU_BUSx domain of Exynos5433 have different base address as
>> > following:
>> > > >>> - CMU_BUS0's base address and range : 0x1360_0000 ~ 0x1360_0b04
>> > > >>> - CMU_BUS1's base address and range : 0x1480_0000 ~ 0x1480_0b04
>> > > >>> - CMU_BUS2's base address and range : 0x1340_0000 ~ 0x1340_0b04
>> > > >>>
>> > > >>> So, I implement CMU_BUSx domain which has each compatible string.
>> > > >
>> > > > But the base address is in the reg property, not in the compatible
>> > > > property. What I mean is to have multiple nodes like
>> > >
>> > > The merged clock driver in mainline have different compatible string
>> > > if base addresss of clock domain is different. So, I implemented each
>> > CMU_BUSx domain
>> > > with different compatible string.
>> >
>> > Why?
>>
>>
>> As I explained on below, each clock domain have different clocks.
>> So, clocks have unique clock name.
>>
>>  If clock driver use only one compatible for various clock domain, clock
>> driver have to know the base address of each domain for distinction of
>> clock domain. I think It is stong dependency between device and driver.
>
> No, not at all. You can have lots of clock controllers with the same
> compatible string defining different instances of the same IP block,
> e.g. for compatible="fixed-clock".

But, "fixed-clock" pass all properties from dt file to
driver/clk/clk-fixed-rate.c.
and "fixed-clock" driver has not the data dependent on h/w. e.g.,
clock offset, parent clock.

>
>> >
>> > > >     clock-controller@113600000 {
>> > > >             reg = <0 0x113600000 0 0x1000>;
>> > > >             compatible = "samsung,exynos5433-cmu";
>> > > >             #clock-cells = <1>;
>> > > >     };
>> > > >
>> > > >     clock-controller@114800000 {
>> > > >             reg = <0 0x114800000 0 0x1000>;
>> > > >             compatible = "samsung,exynos5433-cmu";
>> > > >             #clock-cells = <1>;
>> > > >     };
>> > > >
>> > > > The code will just map the local registers for each instance and then
>> > > > provide the clocks of the right instance when asked for it.
>> > >
>> > > Each clock domain has not the same mux/divider/clock. So, just one
>> > compatible
>> > > string could not support all of clock domains.
>> >
>> > What are the specific differences?
>>
>>
>>
>> > I'm not sure that difference among clock domains because I think it is
>> dependent on the opinion of architect of SoC.
>>
>> cmu_bus0/1/2 are much similar. Just cmu_bus2 has one more mux/gate clock
>> than cmu_bus0/1.
>
> Yes, that's what I mean. You can simply model that extra mux/gate
> in the driver, as long as nothing ever tries to access the clock.

If only use one compatible to support CMU_BUSx domains,
I would implement it as following with Sylwester's guide.

To Sylwester, Tomaz,
Are you agree following method to support CMU_BUSx domains
by using one compatible string?

+/*
+ * Register offset definitions for CMU_BUS{0|1}
+ */
+#define DIV_BUS                0x0600
+#define DIV_STAT_BUS            0x0700
+#define ENABLE_ACLK_BUS            0x0800
+#define ENABLE_PCLK_BUS            0x0900
+#define ENABLE_IP_BUS0            0x0b00
+#define ENABLE_IP_BUS1            0x0b04
+
+#define bus_clk_regs(num)                \
+static unsigned long bus##num_clk_regs[] __initdata = {    \
+    DIV_BUS,                    \
+    DIV_STAT_BUS,                    \
+    ENABLE_ACLK_BUS,                \
+    ENABLE_PCLK_BUS,                \
+    ENABLE_IP_BUS0,                    \
+    ENABLE_IP_BUS1,                    \
+};                            \
+
+#define bus_div_clks(num)                        \
+static struct samsung_div_clock bus##num_div_clks[] __initdata = {    \
+    /* DIV_BUS */                            \
+    DIV(CLK_DIV_PCLK_BUS##num_133, "div_pclk_bus"#num"_133",    \
+            "aclk_bus"#num"_400", DIV_BUS##num, 0, 3),    \
+};                                    \
+
+#define bus_gate_clks(num)                        \
+static struct samsung_gate_clock bus##num_gate_clks[] __initdata = {    \
+    /* ENABLE_ACLK_BUS */                        \
+    GATE(CLK_ACLK_AHB2APB_BUS##num, "aclk_ahb2apb_bus"#num"p",    \
+            "div_pclk_bus"#num"_133", ENABLE_ACLK_BUS##num,    \
+            4, CLK_IGNORE_UNUSED, 0),            \
+    GATE(CLK_ACLK_BUS##numNP_133, "aclk_bus"#num"np_133",        \
+            "div_pclk_bus"##num"_133", ENABLE_ACLK_BUS##num,\
+            2, CLK_IGNORE_UNUSED, 0),            \
+    GATE(CLK_ACLK_BUS##numND_400, "aclk_bus"#num"nd_400",        \
+            "aclk_bus"#num"_400", ENABLE_ACLK_BUS##num,    \
+            0, CLK_IGNORE_UNUSED, 0),            \
+                                    \
+    /* ENABLE_PCLK_BUS */                        \
+    GATE(CLK_PCLK_BUS##numSRVND_133, "pclk_bus"#num"srvnd_133",    \
+            "div_pclk_bus"#num"_133", ENABLE_PCLK_BUS##num,    \
+            2, 0, 0),                    \
+    GATE(CLK_PCLK_PMU_BUS##num, "pclk_pmu_bus"#num,            \
+            "div_pclk_bus"#num"_133", ENABLE_PCLK_BUS##num,    \
+            1, CLK_IGNORE_UNUSED, 0),            \
+    GATE(CLK_PCLK_SYSREG_BUS##num, "pclk_sysreg_bus"#num,        \
+            "div_pclk_bus"#num"_133", ENABLE_PCLK_BUS##num,    \
+            0, 0, 0),                    \
+};                                    \
+
+#define bus_clk_regs(0)
+#define bus_div_clks(0)
+#define bus_gate_clks(0)
+
+#define bus_clk_regs(1)
+#define bus_div_clks(1)
+#define bus_gate_clks(1)
+
+static void __init exynos5433_cmu_bus_init(struct device_node *np)
+{
+    void __iomem *reg_base_bus0, *reg_base_bus1;
+
+    reg_base_bus0 = of_iomap(np, 0);
+    reg_base_bus1 = of_iomap(np, 1);
+
+    bus0_ctx = samsung_clk_init(np, reg_base_bus0, BUS0_NR_CLKS);
+    bus1_ctx = samsung_clk_init(np, reg_base_bus0, BUS0_NR_CLKS);
+
+    samsung_clk_register_div(bus0_ctx, bus0_div_clks,
+                    ARRAY_SIZE(bus0_div_clks));
+    samsung_clk_register_gate(bus0_ctx, bus0_gate_clks,
+                    ARRAY_SIZE(bus0_gate_clks));
+    samsung_clk_register_div(bus1_ctx, bus1_div_clks,
+                    ARRAY_SIZE(bus1_div_clks));
+    samsung_clk_register_gate(bus1_ctx, bus1_gate_clks,
+                    ARRAY_SIZE(bus1_gate_clks));
+
+    samsung_clk_of_provider(np, bus0_ctx);
+    samsung_clk_of_provider(np, bus1_ctx);
+
+}
+CLK_OF_DECLARE(exynos5433_cmu_bus, "samsung,exynos5433-cmu-bus",
+        exynos5433_cmu_bus_init);

Best Regards,
Chanwoo Choi
Arnd Bergmann Nov. 27, 2014, 3:33 p.m. UTC | #10
On Friday 28 November 2014 00:17:40 Chanwoo Choi wrote:
> 
> But, "fixed-clock" pass all properties from dt file to
> driver/clk/clk-fixed-rate.c.
> and "fixed-clock" driver has not the data dependent on h/w. e.g.,
> clock offset, parent clock.

The parent clocks would obviously have to be provided in DT if you
do this. I'm not sure what you mean with clock offsets. What would
it take to describe that?

> >> >
> >> > > >     clock-controller@113600000 {
> >> > > >             reg = <0 0x113600000 0 0x1000>;
> >> > > >             compatible = "samsung,exynos5433-cmu";
> >> > > >             #clock-cells = <1>;
> >> > > >     };
> >> > > >
> >> > > >     clock-controller@114800000 {
> >> > > >             reg = <0 0x114800000 0 0x1000>;
> >> > > >             compatible = "samsung,exynos5433-cmu";
> >> > > >             #clock-cells = <1>;
> >> > > >     };
> >> > > >
> >> > > > The code will just map the local registers for each instance and then
> >> > > > provide the clocks of the right instance when asked for it.
> >> > >
> >> > > Each clock domain has not the same mux/divider/clock. So, just one
> >> > compatible
> >> > > string could not support all of clock domains.
> >> >
> >> > What are the specific differences?
> >>
> >>
> >>
> >> > I'm not sure that difference among clock domains because I think it is
> >> dependent on the opinion of architect of SoC.
> >>
> >> cmu_bus0/1/2 are much similar. Just cmu_bus2 has one more mux/gate clock
> >> than cmu_bus0/1.
> >
> > Yes, that's what I mean. You can simply model that extra mux/gate
> > in the driver, as long as nothing ever tries to access the clock.
> 
> If only use one compatible to support CMU_BUSx domains,
> I would implement it as following with Sylwester's guide.
> 
> To Sylwester, Tomaz,
> Are you agree following method to support CMU_BUSx domains
> by using one compatible string?


> +#define bus_clk_regs(num)                \
> +static unsigned long bus##num_clk_regs[] __initdata = {    \
> +    DIV_BUS,                    \
> +    DIV_STAT_BUS,                    \
> +    ENABLE_ACLK_BUS,                \
> +    ENABLE_PCLK_BUS,                \
> +    ENABLE_IP_BUS0,                    \
> +    ENABLE_IP_BUS1,                    \
> +};                            \

I don't understand why you would need a macro here. Isn't this constant
data that you can pass into multiple devices? The use of macros
definitely makes it worse than the original patch.

> +#define bus_div_clks(num)                        \
> +static struct samsung_div_clock bus##num_div_clks[] __initdata = {    \
> +    /* DIV_BUS */                            \
> +    DIV(CLK_DIV_PCLK_BUS##num_133, "div_pclk_bus"#num"_133",    \
> +            "aclk_bus"#num"_400", DIV_BUS##num, 0, 3),    \
> +};                                    \

To illustrate my point further: CLK_DIV_PCLK_BUS0/1/2 are all the
same, and so are DIV_BUS0/1/2, so you should not need to duplicate
the definitions at all but just call them 'CLK_DIV_PCLK_BUS'
and 'DIV_BUS'.

For the "aclk_bus"#num"_400" and "div_pclk_bus"#num"_133" strings,
I don't know what they refer to. Are you sure they have to be unique?

> +
> +#define bus_clk_regs(0)
> +#define bus_div_clks(0)
> +#define bus_gate_clks(0)
> +
> +#define bus_clk_regs(1)
> +#define bus_div_clks(1)
> +#define bus_gate_clks(1)
> +
> +static void __init exynos5433_cmu_bus_init(struct device_node *np)
> +{
> +    void __iomem *reg_base_bus0, *reg_base_bus1;
> +
> +    reg_base_bus0 = of_iomap(np, 0);
> +    reg_base_bus1 = of_iomap(np, 1);
> +
> +    bus0_ctx = samsung_clk_init(np, reg_base_bus0, BUS0_NR_CLKS);
> +    bus1_ctx = samsung_clk_init(np, reg_base_bus0, BUS0_NR_CLKS);
> +
> +    samsung_clk_register_div(bus0_ctx, bus0_div_clks,
> +                    ARRAY_SIZE(bus0_div_clks));
> +    samsung_clk_register_gate(bus0_ctx, bus0_gate_clks,
> +                    ARRAY_SIZE(bus0_gate_clks));
> +    samsung_clk_register_div(bus1_ctx, bus1_div_clks,
> +                    ARRAY_SIZE(bus1_div_clks));
> +    samsung_clk_register_gate(bus1_ctx, bus1_gate_clks,
> +                    ARRAY_SIZE(bus1_gate_clks));
> +
> +    samsung_clk_of_provider(np, bus0_ctx);
> +    samsung_clk_of_provider(np, bus1_ctx);
> +
> +}
> +CLK_OF_DECLARE(exynos5433_cmu_bus, "samsung,exynos5433-cmu-bus",
> +        exynos5433_cmu_bus_init);

This isn't helpful either: you really have two instances and should
not merge them together into one device node. This should look like

static void __init exynos5433_cmu_bus_init(struct device_node *np)
{
    void __iomem *reg_base_bus;

    reg_base_bus = of_iomap(np, 0);

    bus_ctx = samsung_clk_init(np, reg_base_bus, BUS_NR_CLKS);

    samsung_clk_register_div(bus_ctx, bus_div_clks,
                    ARRAY_SIZE(bus_div_clks));
    samsung_clk_register_gate(bus_ctx, bus_gate_clks,
                    ARRAY_SIZE(bus_gate_clks));

    samsung_clk_of_provider(np, bus0_ctx);
}

and get called three times.

	Arnd
Chanwoo Choi Nov. 27, 2014, 3:44 p.m. UTC | #11
On Fri, Nov 28, 2014 at 12:33 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 28 November 2014 00:17:40 Chanwoo Choi wrote:
>>
>> But, "fixed-clock" pass all properties from dt file to
>> driver/clk/clk-fixed-rate.c.
>> and "fixed-clock" driver has not the data dependent on h/w. e.g.,
>> clock offset, parent clock.
>
> The parent clocks would obviously have to be provided in DT if you
> do this. I'm not sure what you mean with clock offsets. What would
> it take to describe that?
>
>> >> >
>> >> > > >     clock-controller@113600000 {
>> >> > > >             reg = <0 0x113600000 0 0x1000>;
>> >> > > >             compatible = "samsung,exynos5433-cmu";
>> >> > > >             #clock-cells = <1>;
>> >> > > >     };
>> >> > > >
>> >> > > >     clock-controller@114800000 {
>> >> > > >             reg = <0 0x114800000 0 0x1000>;
>> >> > > >             compatible = "samsung,exynos5433-cmu";
>> >> > > >             #clock-cells = <1>;
>> >> > > >     };
>> >> > > >
>> >> > > > The code will just map the local registers for each instance and then
>> >> > > > provide the clocks of the right instance when asked for it.
>> >> > >
>> >> > > Each clock domain has not the same mux/divider/clock. So, just one
>> >> > compatible
>> >> > > string could not support all of clock domains.
>> >> >
>> >> > What are the specific differences?
>> >>
>> >>
>> >>
>> >> > I'm not sure that difference among clock domains because I think it is
>> >> dependent on the opinion of architect of SoC.
>> >>
>> >> cmu_bus0/1/2 are much similar. Just cmu_bus2 has one more mux/gate clock
>> >> than cmu_bus0/1.
>> >
>> > Yes, that's what I mean. You can simply model that extra mux/gate
>> > in the driver, as long as nothing ever tries to access the clock.
>>
>> If only use one compatible to support CMU_BUSx domains,
>> I would implement it as following with Sylwester's guide.
>>
>> To Sylwester, Tomaz,
>> Are you agree following method to support CMU_BUSx domains
>> by using one compatible string?
>
>
>> +#define bus_clk_regs(num)                \
>> +static unsigned long bus##num_clk_regs[] __initdata = {    \
>> +    DIV_BUS,                    \
>> +    DIV_STAT_BUS,                    \
>> +    ENABLE_ACLK_BUS,                \
>> +    ENABLE_PCLK_BUS,                \
>> +    ENABLE_IP_BUS0,                    \
>> +    ENABLE_IP_BUS1,                    \
>> +};                            \
>
> I don't understand why you would need a macro here. Isn't this constant
> data that you can pass into multiple devices? The use of macros
> definitely makes it worse than the original patch.
>
>> +#define bus_div_clks(num)                        \
>> +static struct samsung_div_clock bus##num_div_clks[] __initdata = {    \
>> +    /* DIV_BUS */                            \
>> +    DIV(CLK_DIV_PCLK_BUS##num_133, "div_pclk_bus"#num"_133",    \
>> +            "aclk_bus"#num"_400", DIV_BUS##num, 0, 3),    \
>> +};                                    \
>
> To illustrate my point further: CLK_DIV_PCLK_BUS0/1/2 are all the
> same, and so are DIV_BUS0/1/2, so you should not need to duplicate
> the definitions at all but just call them 'CLK_DIV_PCLK_BUS'
> and 'DIV_BUS'.

CLK_DIV_PCLK_BUS0/1/2 is not all the same.
Each CLK_DIV_PCLK_BUS0/1/2 must need the unique clock number.
Because some device may need some clocks by using unique clock number.

>
> For the "aclk_bus"#num"_400" and "div_pclk_bus"#num"_133" strings,
> I don't know what they refer to. Are you sure they have to be unique?

Sure.
All clocks(mux/divider/gate) must need the unique clock number.

Best Regards,
Chanwoo Choi

>
>> +
>> +#define bus_clk_regs(0)
>> +#define bus_div_clks(0)
>> +#define bus_gate_clks(0)
>> +
>> +#define bus_clk_regs(1)
>> +#define bus_div_clks(1)
>> +#define bus_gate_clks(1)
>> +
>> +static void __init exynos5433_cmu_bus_init(struct device_node *np)
>> +{
>> +    void __iomem *reg_base_bus0, *reg_base_bus1;
>> +
>> +    reg_base_bus0 = of_iomap(np, 0);
>> +    reg_base_bus1 = of_iomap(np, 1);
>> +
>> +    bus0_ctx = samsung_clk_init(np, reg_base_bus0, BUS0_NR_CLKS);
>> +    bus1_ctx = samsung_clk_init(np, reg_base_bus0, BUS0_NR_CLKS);
>> +
>> +    samsung_clk_register_div(bus0_ctx, bus0_div_clks,
>> +                    ARRAY_SIZE(bus0_div_clks));
>> +    samsung_clk_register_gate(bus0_ctx, bus0_gate_clks,
>> +                    ARRAY_SIZE(bus0_gate_clks));
>> +    samsung_clk_register_div(bus1_ctx, bus1_div_clks,
>> +                    ARRAY_SIZE(bus1_div_clks));
>> +    samsung_clk_register_gate(bus1_ctx, bus1_gate_clks,
>> +                    ARRAY_SIZE(bus1_gate_clks));
>> +
>> +    samsung_clk_of_provider(np, bus0_ctx);
>> +    samsung_clk_of_provider(np, bus1_ctx);
>> +
>> +}
>> +CLK_OF_DECLARE(exynos5433_cmu_bus, "samsung,exynos5433-cmu-bus",
>> +        exynos5433_cmu_bus_init);
>
> This isn't helpful either: you really have two instances and should
> not merge them together into one device node. This should look like
>
> static void __init exynos5433_cmu_bus_init(struct device_node *np)
> {
>     void __iomem *reg_base_bus;
>
>     reg_base_bus = of_iomap(np, 0);
>
>     bus_ctx = samsung_clk_init(np, reg_base_bus, BUS_NR_CLKS);
>
>     samsung_clk_register_div(bus_ctx, bus_div_clks,
>                     ARRAY_SIZE(bus_div_clks));
>     samsung_clk_register_gate(bus_ctx, bus_gate_clks,
>                     ARRAY_SIZE(bus_gate_clks));
>
>     samsung_clk_of_provider(np, bus0_ctx);
> }
>
> and get called three times.
>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2014, 3:51 p.m. UTC | #12
On Friday 28 November 2014 00:44:07 Chanwoo Choi wrote:
> >
> >> +#define bus_div_clks(num)                        \
> >> +static struct samsung_div_clock bus##num_div_clks[] __initdata = {    \
> >> +    /* DIV_BUS */                            \
> >> +    DIV(CLK_DIV_PCLK_BUS##num_133, "div_pclk_bus"#num"_133",    \
> >> +            "aclk_bus"#num"_400", DIV_BUS##num, 0, 3),    \
> >> +};                                    \
> >
> > To illustrate my point further: CLK_DIV_PCLK_BUS0/1/2 are all the
> > same, and so are DIV_BUS0/1/2, so you should not need to duplicate
> > the definitions at all but just call them 'CLK_DIV_PCLK_BUS'
> > and 'DIV_BUS'.
> 
> CLK_DIV_PCLK_BUS0/1/2 is not all the same.
> Each CLK_DIV_PCLK_BUS0/1/2 must need the unique clock number.
> Because some device may need some clocks by using unique clock number.

This is from your original patch:

+/* CMU_BUS0 */
+#define CLK_DIV_PCLK_BUS0_133                          1
+
+#define CLK_ACLK_AHB2APB_BUS0P                         2
+#define CLK_ACLK_BUS0NP_133                            3
+#define CLK_ACLK_BUS0ND_400                            4
+#define CLK_PCLK_BUS0SRVND_133                         5
+#define CLK_PCLK_PMU_BUS0                              6
+#define CLK_PCLK_SYSREG_BUS0                           7
+
+#define BUS0_NR_CLK                                    8
+
+/* CMU_BUS1 */
+#define CLK_DIV_PCLK_BUS1_133                          1
+
+#define CLK_ACLK_AHB2APB_BUS1P                         2
+#define CLK_ACLK_BUS1NP_133                            3
+#define CLK_ACLK_BUS1ND_400                            4
+#define CLK_PCLK_BUS1SRVND_133                         5
+#define CLK_PCLK_PMU_BUS1                              6
+#define CLK_PCLK_SYSREG_BUS1                           7
+
+#define BUS1_NR_CLK                                    8
+
+/* CMU_BUS2 */
+#define CLK_MOUT_ACLK_BUS2_400_USER                    1
+
+#define CLK_DIV_PCLK_BUS2_133                          2
+
+#define CLK_ACLK_AHB2APB_BUS2P                         3
+#define CLK_ACLK_BUS2NP_133                            4
+#define CLK_ACLK_BUS2BEND_400                          5
+#define CLK_ACLK_BUS2RTND_400                          6
+#define CLK_PCLK_BUS2SRVND_133                         7
+#define CLK_PCLK_PMU_BUS2                              8
+#define CLK_PCLK_SYSREG_BUS2                           9
+
+#define BUS2_NR_CLK                                    10


The numbers are arbitrarily assigned, but for bus0 and bus1,
they are all identical, while bus2 uses a lightly different
numbering, which you could easily change, e.g. by using the
numbers you have for bus2 on bus0 and bus1 as well.

+ * Register offset definitions for CMU_BUS0
+ */
+#define DIV_BUS0                       0x0600
+#define DIV_STAT_BUS0                  0x0700
+#define ENABLE_ACLK_BUS0               0x0800
+#define ENABLE_PCLK_BUS0               0x0900
+#define ENABLE_IP_BUS0                 0x0b00
+#define ENABLE_IP_BUS1                 0x0b04
+

+/*
+ * Register offset definitions for CMU_BUS1
+ */
+#define DIV_BUS1                       0x0600
+#define DIV_STAT_BUS1                  0x0700
+#define ENABLE_ACLK_BUS1               0x0800
+#define ENABLE_PCLK_BUS1               0x0900
+#define ENABLE_IP_BUS10                        0x0b00
+#define ENABLE_IP_BUS11                        0x0b04

+/*
+ * Register offset definitions for CMU_BUS2
+ */
+#define MUX_SEL_BUS2                   0x0200
+#define MUX_ENABLE_BUS2                        0x0300
+#define MUX_STAT_BUS2                  0x0400
+#define DIV_BUS2                       0x0600
+#define DIV_STAT_BUS2                  0x0700
+#define ENABLE_ACLK_BUS2               0x0800
+#define ENABLE_PCLK_BUS2               0x0900
+#define ENABLE_IP_BUS20                        0x0b00
+#define ENABLE_IP_BUS21                        0x0b04

More importantly, the register offsets are all identical, except that
bus2 has the additional MUX_SEL and MUX_ENABLE definitions. It's very
obvious that this is the same hardware block in multiple instances.

	Arnd
Chanwoo Choi Nov. 27, 2014, 3:58 p.m. UTC | #13
On Fri, Nov 28, 2014 at 12:51 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 28 November 2014 00:44:07 Chanwoo Choi wrote:
>> >
>> >> +#define bus_div_clks(num)                        \
>> >> +static struct samsung_div_clock bus##num_div_clks[] __initdata = {    \
>> >> +    /* DIV_BUS */                            \
>> >> +    DIV(CLK_DIV_PCLK_BUS##num_133, "div_pclk_bus"#num"_133",    \
>> >> +            "aclk_bus"#num"_400", DIV_BUS##num, 0, 3),    \
>> >> +};                                    \
>> >
>> > To illustrate my point further: CLK_DIV_PCLK_BUS0/1/2 are all the
>> > same, and so are DIV_BUS0/1/2, so you should not need to duplicate
>> > the definitions at all but just call them 'CLK_DIV_PCLK_BUS'
>> > and 'DIV_BUS'.
>>
>> CLK_DIV_PCLK_BUS0/1/2 is not all the same.
>> Each CLK_DIV_PCLK_BUS0/1/2 must need the unique clock number.
>> Because some device may need some clocks by using unique clock number.
>
> This is from your original patch:
>
> +/* CMU_BUS0 */
> +#define CLK_DIV_PCLK_BUS0_133                          1
> +
> +#define CLK_ACLK_AHB2APB_BUS0P                         2
> +#define CLK_ACLK_BUS0NP_133                            3
> +#define CLK_ACLK_BUS0ND_400                            4
> +#define CLK_PCLK_BUS0SRVND_133                         5
> +#define CLK_PCLK_PMU_BUS0                              6
> +#define CLK_PCLK_SYSREG_BUS0                           7
> +
> +#define BUS0_NR_CLK                                    8
> +
> +/* CMU_BUS1 */
> +#define CLK_DIV_PCLK_BUS1_133                          1
> +
> +#define CLK_ACLK_AHB2APB_BUS1P                         2
> +#define CLK_ACLK_BUS1NP_133                            3
> +#define CLK_ACLK_BUS1ND_400                            4
> +#define CLK_PCLK_BUS1SRVND_133                         5
> +#define CLK_PCLK_PMU_BUS1                              6
> +#define CLK_PCLK_SYSREG_BUS1                           7
> +
> +#define BUS1_NR_CLK                                    8
> +
> +/* CMU_BUS2 */
> +#define CLK_MOUT_ACLK_BUS2_400_USER                    1
> +
> +#define CLK_DIV_PCLK_BUS2_133                          2
> +
> +#define CLK_ACLK_AHB2APB_BUS2P                         3
> +#define CLK_ACLK_BUS2NP_133                            4
> +#define CLK_ACLK_BUS2BEND_400                          5
> +#define CLK_ACLK_BUS2RTND_400                          6
> +#define CLK_PCLK_BUS2SRVND_133                         7
> +#define CLK_PCLK_PMU_BUS2                              8
> +#define CLK_PCLK_SYSREG_BUS2                           9
> +
> +#define BUS2_NR_CLK                                    10
>
>
> The numbers are arbitrarily assigned, but for bus0 and bus1,
> they are all identical, while bus2 uses a lightly different
> numbering, which you could easily change, e.g. by using the
> numbers you have for bus2 on bus0 and bus1 as well.

OK, I'll remove duplicate clock number due to the same on other clock domain.

If use only CLK_DIV_PCLK_BUS instead of CLK_DIV_PCLK_BUS0/1/2,
I think I have to use three compatible string for each CMU_BUSx domain
as following:.

clocks = <&cmu_bus0 CLK_DIV_PCLK_BUS>;
clocks = <&cmu_bus1 CLK_DIV_PCLK_BUS>;
clocks = <&cmu_bus2 CLK_DIV_PCLK_BUS>;

>
> + * Register offset definitions for CMU_BUS0
> + */
> +#define DIV_BUS0                       0x0600
> +#define DIV_STAT_BUS0                  0x0700
> +#define ENABLE_ACLK_BUS0               0x0800
> +#define ENABLE_PCLK_BUS0               0x0900
> +#define ENABLE_IP_BUS0                 0x0b00
> +#define ENABLE_IP_BUS1                 0x0b04
> +
>
> +/*
> + * Register offset definitions for CMU_BUS1
> + */
> +#define DIV_BUS1                       0x0600
> +#define DIV_STAT_BUS1                  0x0700
> +#define ENABLE_ACLK_BUS1               0x0800
> +#define ENABLE_PCLK_BUS1               0x0900
> +#define ENABLE_IP_BUS10                        0x0b00
> +#define ENABLE_IP_BUS11                        0x0b04
>
> +/*
> + * Register offset definitions for CMU_BUS2
> + */
> +#define MUX_SEL_BUS2                   0x0200
> +#define MUX_ENABLE_BUS2                        0x0300
> +#define MUX_STAT_BUS2                  0x0400
> +#define DIV_BUS2                       0x0600
> +#define DIV_STAT_BUS2                  0x0700
> +#define ENABLE_ACLK_BUS2               0x0800
> +#define ENABLE_PCLK_BUS2               0x0900
> +#define ENABLE_IP_BUS20                        0x0b00
> +#define ENABLE_IP_BUS21                        0x0b04
>
> More importantly, the register offsets are all identical, except that
> bus2 has the additional MUX_SEL and MUX_ENABLE definitions. It's very
> obvious that this is the same hardware block in multiple instances.

You're right. I'll remove duplicate offset definition.

Best Regards,
Chanwoo Choi
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/exynos5433-clock.txt b/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
index 9a6ae75..03ae40a 100644
--- a/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
+++ b/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
@@ -25,6 +25,9 @@  Required Properties:
     which generates clocks for Display (DECON/HDMI/DSIM/MIXER) IPs.
   - "samsung,exynos5433-cmu-aud"   - clock controller compatible for CMU_AUD
     which generates clocks for Cortex-A5/BUS/AUDIO clocks.
+  - "samsung,exynos5433-cmu-bus0", "samsung,exynos5433-cmu-bus1"
+    and "samsung,exynos5433-cmu-bus2" - clock controller compatible for CMU_BUS
+    which generates global data buses clock and global peripheral buses clock.
 
 - reg: physical base address of the controller and length of memory mapped
   region.
@@ -94,6 +97,24 @@  Example 1: Examples of clock controller nodes are listed below.
 		#clock-cells = <1>;
 	};
 
+	cmu_bus0: clock-controller@0x13600000 {
+		compatible = "samsung,exynos5433-cmu-bus0";
+		reg = <0x13600000 0x0b04>;
+		#clock-cells = <1>;
+	};
+
+	cmu_bus1: clock-controller@0x14800000 {
+		compatible = "samsung,exynos5433-cmu-bus1";
+		reg = <0x14800000 0x0b04>;
+		#clock-cells = <1>;
+	};
+
+	cmu_bus2: clock-controller@0x13400000 {
+		compatible = "samsung,exynos5433-cmu-bus2";
+		reg = <0x13400000 0x0b04>;
+		#clock-cells = <1>;
+	};
+
 Example 2: UART controller node that consumes the clock generated by the clock
 	   controller.
 
diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 9f28672..f0975e1 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -428,7 +428,7 @@  static struct samsung_div_clock top_div_clks[] __initdata = {
 			DIV_TOP2, 0, 3),
 
 	/* DIV_TOP3 */
-	DIV(CLK_DIV_ACLK_IMEM_SSSX, "div_aclk_imem_sssx",
+	DIV(CLK_DIV_ACLK_IMEM_SSSX_266, "div_aclk_imem_sssx_266",
 			"mout_bus_pll_user", DIV_TOP3, 24, 3),
 	DIV(CLK_DIV_ACLK_IMEM_200, "div_aclk_imem_200",
 			"mout_bus_pll_user", DIV_TOP3, 20, 3),
@@ -443,6 +443,14 @@  static struct samsung_div_clock top_div_clks[] __initdata = {
 	DIV(CLK_DIV_ACLK_PERIS_66_A, "div_aclk_peris_66_a",
 			"mout_bus_pll_user", DIV_TOP3, 0, 3),
 
+	/* DIV_TOP4 */
+	DIV(CLK_DIV_ACLK_G3D_400, "div_aclk_g3d_400", "mout_bus_pll_user",
+			DIV_TOP4, 8, 3),
+	DIV(CLK_DIV_ACLK_BUS0_400, "div_aclk_bus0_400", "mout_aclk_bus0_400",
+			DIV_TOP4, 4, 3),
+	DIV(CLK_DIV_ACLK_BUS1_400, "div_aclk_bus1_400", "mout_bus_pll_user",
+			DIV_TOP4, 0, 3),
+
 	/* DIV_TOP_FSYS0 */
 	DIV(CLK_DIV_SCLK_MMC1_B, "div_sclk_mmc1_b", "div_sclk_mmc1_a",
 			DIV_TOP_FSYS0, 16, 8),
@@ -506,6 +514,19 @@  static struct samsung_div_clock top_div_clks[] __initdata = {
 
 static struct samsung_gate_clock top_gate_clks[] __initdata = {
 	/* ENABLE_ACLK_TOP */
+	GATE(CLK_ACLK_G3D_400, "aclk_g3d_400", "div_aclk_g3d_400",
+			ENABLE_ACLK_TOP, 30, 0, 0),
+	GATE(CLK_ACLK_IMEM_SSX_266, "aclk_imem_ssx_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",
+			ENABLE_ACLK_TOP, 26, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS1_400, "aclk_bus1_400", "div_aclk_bus1_400",
+			ENABLE_ACLK_TOP, 25, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_IMEM_200, "aclk_imem_200", "div_aclk_imem_266",
+			ENABLE_ACLK_TOP, 24, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_IMEM_266, "aclk_imem_266", "div_aclk_imem_200",
+			ENABLE_ACLK_TOP, 23, CLK_IGNORE_UNUSED, 0),
 	GATE(CLK_ACLK_PERIC_66, "aclk_peric_66", "div_aclk_peric_66_b",
 			ENABLE_ACLK_TOP, 22, CLK_IGNORE_UNUSED, 0),
 	GATE(CLK_ACLK_PERIS_66, "aclk_peris_66", "div_aclk_peris_66_b",
@@ -2629,3 +2650,205 @@  static void __init exynos5433_cmu_aud_init(struct device_node *np)
 }
 CLK_OF_DECLARE(exynos5433_cmu_aud, "samsung,exynos5433-cmu-aud",
 		exynos5433_cmu_aud_init);
+
+
+/*
+ * Register offset definitions for CMU_BUS0
+ */
+#define DIV_BUS0			0x0600
+#define DIV_STAT_BUS0			0x0700
+#define ENABLE_ACLK_BUS0		0x0800
+#define ENABLE_PCLK_BUS0		0x0900
+#define ENABLE_IP_BUS0			0x0b00
+#define ENABLE_IP_BUS1			0x0b04
+
+static unsigned long bus0_clk_regs[] __initdata = {
+	DIV_BUS0,
+	DIV_STAT_BUS0,
+	ENABLE_ACLK_BUS0,
+	ENABLE_PCLK_BUS0,
+	ENABLE_IP_BUS0,
+	ENABLE_IP_BUS1,
+};
+
+static struct samsung_div_clock bus0_div_clks[] __initdata = {
+	/* DIV_BUS0 */
+	DIV(CLK_DIV_PCLK_BUS0_133, "div_pclk_bus0_133", "aclk_bus0_400",
+			DIV_BUS0, 0, 3),
+};
+
+static struct samsung_gate_clock bus0_gate_clks[] __initdata = {
+	/* ENABLE_ACLK_BUS0 */
+	GATE(CLK_ACLK_AHB2APB_BUS0P, "aclk_ahb2apb_bus0p", "div_pclk_bus0_133",
+			ENABLE_ACLK_BUS0, 4, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS0NP_133, "aclk_bus0np_133", "div_pclk_bus0_133",
+			ENABLE_ACLK_BUS0, 2, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS0ND_400, "aclk_bus0nd_400", "aclk_bus0_400",
+			ENABLE_ACLK_BUS0, 0, CLK_IGNORE_UNUSED, 0),
+
+	/* ENABLE_PCLK_BUS0 */
+	GATE(CLK_PCLK_BUS0SRVND_133, "pclk_bus0srvnd_133", "div_pclk_bus0_133",
+			ENABLE_PCLK_BUS0, 2, 0, 0),
+	GATE(CLK_PCLK_PMU_BUS0, "pclk_pmu_bus0", "div_pclk_bus0_133",
+			ENABLE_PCLK_BUS0, 1, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_PCLK_SYSREG_BUS0, "pclk_sysreg_bus0", "div_pclk_bus0_133",
+			ENABLE_PCLK_BUS0, 0, 0, 0),
+};
+
+static struct samsung_cmu_info bus0_cmu_info __initdata = {
+	.div_clks		= bus0_div_clks,
+	.nr_div_clks		= ARRAY_SIZE(bus0_div_clks),
+	.gate_clks		= bus0_gate_clks,
+	.nr_gate_clks		= ARRAY_SIZE(bus0_gate_clks),
+	.nr_clk_ids		= BUS0_NR_CLK,
+	.clk_regs		= bus0_clk_regs,
+	.nr_clk_regs		= ARRAY_SIZE(bus0_clk_regs),
+};
+
+static void __init exynos5433_cmu_bus0_init(struct device_node *np)
+{
+	samsung_cmu_register_one(np, &bus0_cmu_info);
+}
+CLK_OF_DECLARE(exynos5433_cmu_bus0, "samsung,exynos5433-cmu-bus0",
+		exynos5433_cmu_bus0_init);
+
+/*
+ * Register offset definitions for CMU_BUS1
+ */
+#define DIV_BUS1			0x0600
+#define DIV_STAT_BUS1			0x0700
+#define ENABLE_ACLK_BUS1		0x0800
+#define ENABLE_PCLK_BUS1		0x0900
+#define ENABLE_IP_BUS10			0x0b00
+#define ENABLE_IP_BUS11			0x0b04
+
+static unsigned long bus1_clk_regs[] __initdata = {
+	DIV_BUS1,
+	DIV_STAT_BUS1,
+	ENABLE_ACLK_BUS1,
+	ENABLE_PCLK_BUS1,
+	ENABLE_IP_BUS10,
+	ENABLE_IP_BUS11,
+};
+
+static struct samsung_div_clock bus1_div_clks[] __initdata = {
+	/* DIV_BUS1 */
+	DIV(CLK_DIV_PCLK_BUS1_133, "div_pclk_bus1_133", "aclk_bus1_400",
+			DIV_BUS1, 0, 3),
+};
+
+static struct samsung_gate_clock bus1_gate_clks[] __initdata = {
+	/* ENABLE_ACLK_BUS1 */
+	GATE(CLK_ACLK_AHB2APB_BUS1P, "aclk_ahb2apb_bus1p", "div_pclk_bus1_133",
+			ENABLE_ACLK_BUS1, 4, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS1NP_133, "aclk_bus1np_133", "div_pclk_bus1_133",
+			ENABLE_ACLK_BUS1, 2, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS1ND_400, "aclk_bus1nd_400", "aclk_bus1_400",
+			ENABLE_ACLK_BUS1, 0, CLK_IGNORE_UNUSED, 0),
+
+	/* ENABLE_PCLK_BUS1 */
+	GATE(CLK_PCLK_BUS1SRVND_133, "pclk_bus1srvnd_133", "div_pclk_bus1_133",
+			ENABLE_PCLK_BUS1, 2, 0, 0),
+	GATE(CLK_PCLK_PMU_BUS1, "pclk_pmu_bus1", "div_pclk_bus1_133",
+			ENABLE_PCLK_BUS1, 1, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_PCLK_SYSREG_BUS1, "pclk_sysreg_bus1", "div_pclk_bus1_133",
+			ENABLE_PCLK_BUS1, 0, 0, 0),
+};
+
+static struct samsung_cmu_info bus1_cmu_info __initdata = {
+	.div_clks		= bus1_div_clks,
+	.nr_div_clks		= ARRAY_SIZE(bus1_div_clks),
+	.gate_clks		= bus1_gate_clks,
+	.nr_gate_clks		= ARRAY_SIZE(bus1_gate_clks),
+	.nr_clk_ids		= BUS1_NR_CLK,
+	.clk_regs		= bus1_clk_regs,
+	.nr_clk_regs		= ARRAY_SIZE(bus1_clk_regs),
+};
+
+static void __init exynos5433_cmu_bus1_init(struct device_node *np)
+{
+	samsung_cmu_register_one(np, &bus1_cmu_info);
+}
+CLK_OF_DECLARE(exynos5433_cmu_bus1, "samsung,exynos5433-cmu-bus1",
+		exynos5433_cmu_bus1_init);
+
+/*
+ * Register offset definitions for CMU_BUS2
+ */
+#define MUX_SEL_BUS2			0x0200
+#define MUX_ENABLE_BUS2			0x0300
+#define MUX_STAT_BUS2			0x0400
+#define DIV_BUS2			0x0600
+#define DIV_STAT_BUS2			0x0700
+#define ENABLE_ACLK_BUS2		0x0800
+#define ENABLE_PCLK_BUS2		0x0900
+#define ENABLE_IP_BUS20			0x0b00
+#define ENABLE_IP_BUS21			0x0b04
+
+static unsigned long bus2_clk_regs[] __initdata = {
+	MUX_SEL_BUS2,
+	MUX_ENABLE_BUS2,
+	MUX_STAT_BUS2,
+	DIV_BUS2,
+	DIV_STAT_BUS2,
+	ENABLE_ACLK_BUS2,
+	ENABLE_PCLK_BUS2,
+	ENABLE_IP_BUS20,
+	ENABLE_IP_BUS21,
+};
+
+/* list of all parent clock list */
+PNAME(mout_aclk_bus2_400_p)	= { "fin_pll", "aclk_bus2_400", };
+
+static struct samsung_mux_clock bus2_mux_clks[] __initdata = {
+	/* MUX_SEL_BUS2 */
+	MUX(CLK_MOUT_ACLK_BUS2_400_USER, "mout_aclk_bus2_400_user",
+			mout_aclk_bus2_400_p, MUX_SEL_BUS2, 0, 1),
+};
+
+static struct samsung_div_clock bus2_div_clks[] __initdata = {
+	/* DIV_BUS2 */
+	DIV(CLK_DIV_PCLK_BUS2_133, "div_pclk_bus2_133",
+			"mout_aclk_bus2_400_user", DIV_BUS2, 0, 3),
+};
+
+static struct samsung_gate_clock bus2_gate_clks[] __initdata = {
+	/* ENABLE_ACLK_BUS2 */
+	GATE(CLK_ACLK_AHB2APB_BUS2P, "aclk_ahb2apb_bus2p", "div_pclk_bus2_133",
+			ENABLE_ACLK_BUS2, 3, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS2NP_133, "aclk_bus2np_133", "div_pclk_bus2_133",
+			ENABLE_ACLK_BUS2, 2, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS2BEND_400, "aclk_bus2bend_400",
+			"mout_aclk_bus2_400_user", ENABLE_ACLK_BUS2,
+			1, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_BUS2RTND_400, "aclk_bus2rtnd_400",
+			"mout_aclk_bus2_400_user", ENABLE_ACLK_BUS2,
+			0, CLK_IGNORE_UNUSED, 0),
+
+	/* ENABLE_PCLK_BUS2 */
+	GATE(CLK_PCLK_BUS2SRVND_133, "pclk_bus2srvnd_133", "div_pclk_bus2_133",
+			ENABLE_PCLK_BUS2, 2, 0, 0),
+	GATE(CLK_PCLK_PMU_BUS2, "pclk_pmu_bus2", "div_pclk_bus2_133",
+			ENABLE_PCLK_BUS2, 1, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_PCLK_SYSREG_BUS2, "pclk_sysreg_bus2", "div_pclk_bus2_133",
+			ENABLE_PCLK_BUS2, 0, 0, 0),
+};
+
+static struct samsung_cmu_info bus2_cmu_info __initdata = {
+	.mux_clks		= bus2_mux_clks,
+	.nr_mux_clks		= ARRAY_SIZE(bus2_mux_clks),
+	.div_clks		= bus2_div_clks,
+	.nr_div_clks		= ARRAY_SIZE(bus2_div_clks),
+	.gate_clks		= bus2_gate_clks,
+	.nr_gate_clks		= ARRAY_SIZE(bus2_gate_clks),
+	.nr_clk_ids		= BUS2_NR_CLK,
+	.clk_regs		= bus2_clk_regs,
+	.nr_clk_regs		= ARRAY_SIZE(bus2_clk_regs),
+};
+
+static void __init exynos5433_cmu_bus2_init(struct device_node *np)
+{
+	samsung_cmu_register_one(np, &bus2_cmu_info);
+}
+CLK_OF_DECLARE(exynos5433_cmu_bus2, "samsung,exynos5433-cmu-bus2",
+		exynos5433_cmu_bus2_init);
diff --git a/include/dt-bindings/clock/exynos5433.h b/include/dt-bindings/clock/exynos5433.h
index e1c848a..56eb8c8 100644
--- a/include/dt-bindings/clock/exynos5433.h
+++ b/include/dt-bindings/clock/exynos5433.h
@@ -72,7 +72,7 @@ 
 #define CLK_MOUT_SCLK_HDMI_SPDIF	64
 
 #define CLK_DIV_ACLK_FSYS_200		100
-#define CLK_DIV_ACLK_IMEM_SSSX		101
+#define CLK_DIV_ACLK_IMEM_SSSX_266	101
 #define CLK_DIV_ACLK_IMEM_200		102
 #define CLK_DIV_ACLK_IMEM_266		103
 #define CLK_DIV_ACLK_PERIC_66_B		104
@@ -108,6 +108,9 @@ 
 #define CLK_DIV_ACLK_MFC_400		134
 #define CLK_DIV_ACLK_G2D_266		135
 #define CLK_DIV_ACLK_G2D_400		136
+#define CLK_DIV_ACLK_G3D_400		137
+#define CLK_DIV_ACLK_BUS0_400		138
+#define CLK_DIV_ACLK_BUS1_400		139
 
 #define CLK_ACLK_PERIC_66		200
 #define CLK_ACLK_PERIS_66		201
@@ -131,8 +134,14 @@ 
 #define CLK_SCLK_AUDIO0			219
 #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_BUS0_400		224
+#define CLK_ACLK_BUS1_400		225
+#define CLK_ACLK_IMEM_200		226
+#define CLK_ACLK_IMEM_266		227
 
-#define TOP_NR_CLK			222
+#define TOP_NR_CLK			228
 
 /* CMU_CPIF */
 #define CLK_FOUT_MPHY_PLL		1
@@ -680,4 +689,43 @@ 
 
 #define AUD_NR_CLK					48
 
+/* CMU_BUS0 */
+#define CLK_DIV_PCLK_BUS0_133				1
+
+#define CLK_ACLK_AHB2APB_BUS0P				2
+#define CLK_ACLK_BUS0NP_133				3
+#define CLK_ACLK_BUS0ND_400				4
+#define CLK_PCLK_BUS0SRVND_133				5
+#define CLK_PCLK_PMU_BUS0				6
+#define CLK_PCLK_SYSREG_BUS0				7
+
+#define BUS0_NR_CLK					8
+
+/* CMU_BUS1 */
+#define CLK_DIV_PCLK_BUS1_133				1
+
+#define CLK_ACLK_AHB2APB_BUS1P				2
+#define CLK_ACLK_BUS1NP_133				3
+#define CLK_ACLK_BUS1ND_400				4
+#define CLK_PCLK_BUS1SRVND_133				5
+#define CLK_PCLK_PMU_BUS1				6
+#define CLK_PCLK_SYSREG_BUS1				7
+
+#define BUS1_NR_CLK					8
+
+/* CMU_BUS2 */
+#define CLK_MOUT_ACLK_BUS2_400_USER			1
+
+#define CLK_DIV_PCLK_BUS2_133				2
+
+#define CLK_ACLK_AHB2APB_BUS2P				3
+#define CLK_ACLK_BUS2NP_133				4
+#define CLK_ACLK_BUS2BEND_400				5
+#define CLK_ACLK_BUS2RTND_400				6
+#define CLK_PCLK_BUS2SRVND_133				7
+#define CLK_PCLK_PMU_BUS2				8
+#define CLK_PCLK_SYSREG_BUS2				9
+
+#define BUS2_NR_CLK					10
+
 #endif /* _DT_BINDINGS_CLOCK_EXYNOS5433_H */