[PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[Jiancheng Xue]

add dt-binding document and header file for hi3519 clock

Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
---
 .../devicetree/bindings/clock/hi3519-crg.txt       | 46 +++++++++++++++++++
 include/dt-bindings/clock/hi3519-clock.h           | 51 ++++++++++++++++++++++
 2 files changed, 97 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
 create mode 100644 include/dt-bindings/clock/hi3519-clock.h

diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
new file mode 100644
index 0000000..e0d30a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
@@ -0,0 +1,46 @@
+* Hisilicon Hi3519 Clock and Reset Generator(CRG)
+
+The Hi3519 CRG module provides clock and reset signals to various
+controllers within the SoC.
+
+This binding uses the following bindings:
+    Documentation/devicetree/bindings/clock/clock-bindings.txt
+    Documentation/devicetree/bindings/reset/reset.txt
+
+Required Properties:
+
+- compatible: should be one of the following.
+  - "hisilicon,hi3519-crg" - controller compatible with Hi3519 SoC.
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- #clock-cells: should be 1.
+
+Each clock is assigned an identifier and client nodes use this identifier
+to specify the clock which they consume.
+
+All these identifier could be found in <dt-bindings/clock/hi3519-clock.h>.
+
+- #reset-cells: should be 2.
+
+A reset signal can be controlled by writing a bit register in the CRG module.
+The reset specifier consists of two cells. The first cell represents the
+register offset relative to the base address. The second cell represents the
+bit index in the register.
+
+Example: CRG nodes
+CRG: clock-reset-controller {
+	compatible = "hisilicon,hi3519-crg";
+        reg = <0x12010000 0x10000>;
+        #clock-cells = <1>;
+        #reset-cells = <2>;
+};
+
+Example: consumer nodes
+i2c0: i2c@0x12110000 {
+	compatible = "hisilicon,hi3519-i2c";
+        reg = <0x12110000 0x1000>;
+        clocks = <&CRG HI3519_I2C0_RST>;*/
+        resets = <&CRG 0xE4 0>;
+};
diff --git a/include/dt-bindings/clock/hi3519-clock.h b/include/dt-bindings/clock/hi3519-clock.h
new file mode 100644
index 0000000..89c0d5e
--- /dev/null
+++ b/include/dt-bindings/clock/hi3519-clock.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __DTS_HI3519_CLOCK_H
+#define __DTS_HI3519_CLOCK_H
+
+/* fixed rate */
+#define HI3519_FIXED_400M		1
+#define HI3519_FIXED_200M		2
+#define HI3519_FIXED_125M		3
+#define HI3519_FIXED_150M		4
+#define HI3519_FIXED_75M		5
+#define HI3519_FIXED_300M		6
+#define HI3519_FIXED_50M		7
+#define HI3519_FIXED_24M		8
+#define HI3519_FIXED_3M			9
+
+/* mux clocks */
+#define HI3519_FMC_MUX			32
+#define HI3519_I2C_MUX			33
+#define HI3519_UART_MUX			34
+#define HI3519_SYSAXI_MUX		35
+
+/*fixed factor clocks*/
+#define HI3519_SYSAPB_CLK		64
+
+/* gate clocks */
+#define HI3519_FMC_CLK			129
+#define HI3519_UART0_CLK		153
+#define HI3519_UART1_CLK		154
+#define HI3519_UART2_CLK		155
+#define HI3519_UART3_CLK		156
+#define HI3519_UART4_CLK		157
+
+#define HI3519_NR_CLKS			256
+#define HI3519_NR_RSTS			256
+#endif	/* __DTS_HI3519_CLOCK_H */
-- 
1.9.1

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[Arnd Bergmann]

On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
> +#ifndef __DTS_HI3519_CLOCK_H
> +#define __DTS_HI3519_CLOCK_H

Please try to avoid adding headers like this if you can at all.

I might ask you to merge the header file in one merge window
otherwise and submit the platform code one kernel later, as they
tendn to cause us needless dependencies otherwise.


> +/* fixed rate */
> +#define HI3519_FIXED_400M              1
> +#define HI3519_FIXED_200M              2
> +#define HI3519_FIXED_125M              3
> +#define HI3519_FIXED_150M              4
> +#define HI3519_FIXED_75M               5
> +#define HI3519_FIXED_300M              6
> +#define HI3519_FIXED_50M               7
> +#define HI3519_FIXED_24M               8
> +#define HI3519_FIXED_3M                        9
> +
> +/* mux clocks */
> +#define HI3519_FMC_MUX                 32
> +#define HI3519_I2C_MUX                 33
> +#define HI3519_UART_MUX                        34
> +#define HI3519_SYSAXI_MUX              35
> +
> +/*fixed factor clocks*/
> +#define HI3519_SYSAPB_CLK              64
> +
> +/* gate clocks */
> +#define HI3519_FMC_CLK                 129
> +#define HI3519_UART0_CLK               153
> +#define HI3519_UART1_CLK               154
> +#define HI3519_UART2_CLK               155
> +#define HI3519_UART3_CLK               156
> +#define HI3519_UART4_CLK               157

Where do those numbers come from? They are not consecutive, so it sounds
like they are directly from the data sheet and won't be needed in the driver.
If that's true, just use the numbers directly, as you do for everything
else.

> +#define HI3519_NR_CLKS                 256
> +#define HI3519_NR_RSTS                 256
> 
These seem to not be needed at all.

	Arnd

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

Hi Arnd,

On 2015/12/3 17:44, Arnd Bergmann wrote:
> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>> +#ifndef __DTS_HI3519_CLOCK_H
>> +#define __DTS_HI3519_CLOCK_H
> 
> Please try to avoid adding headers like this if you can at all.
> 
> I might ask you to merge the header file in one merge window
> otherwise and submit the platform code one kernel later, as they
> tendn to cause us needless dependencies otherwise.
> 

Sorry. In v1, Rob suggested putting binding doc and header files in
a separate patch. The clock driver indeed depends on the header.

I will put the header and the clock driver in a patch, and keep the
binding doc in another patch.

> 
>> +/* fixed rate */
>> +#define HI3519_FIXED_400M              1
>> +#define HI3519_FIXED_200M              2
>> +#define HI3519_FIXED_125M              3
>> +#define HI3519_FIXED_150M              4
>> +#define HI3519_FIXED_75M               5
>> +#define HI3519_FIXED_300M              6
>> +#define HI3519_FIXED_50M               7
>> +#define HI3519_FIXED_24M               8
>> +#define HI3519_FIXED_3M                        9
>> +
>> +/* mux clocks */
>> +#define HI3519_FMC_MUX                 32
>> +#define HI3519_I2C_MUX                 33
>> +#define HI3519_UART_MUX                        34
>> +#define HI3519_SYSAXI_MUX              35
>> +
>> +/*fixed factor clocks*/
>> +#define HI3519_SYSAPB_CLK              64
>> +
>> +/* gate clocks */
>> +#define HI3519_FMC_CLK                 129
>> +#define HI3519_UART0_CLK               153
>> +#define HI3519_UART1_CLK               154
>> +#define HI3519_UART2_CLK               155
>> +#define HI3519_UART3_CLK               156
>> +#define HI3519_UART4_CLK               157
> 
> Where do those numbers come from? They are not consecutive, so it sounds
> like they are directly from the data sheet and won't be needed in the driver.
> If that's true, just use the numbers directly, as you do for everything
> else.

The numbers are defined by myself, not directly from the data sheet. Some numbers
are reserved for device nodes which will be added later. So they are not consecutive now.

> 
>> +#define HI3519_NR_CLKS                 256
>> +#define HI3519_NR_RSTS                 256
>>
> These seem to not be needed at all.

These are used in drivers/clk/hisilicon/clk-hi3519.c.

> 
> 	Arnd
> 
> .
> 

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[Arnd Bergmann]

On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
> Hi Arnd,
> 
> On 2015/12/3 17:44, Arnd Bergmann wrote:
> > On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
> >> +#ifndef __DTS_HI3519_CLOCK_H
> >> +#define __DTS_HI3519_CLOCK_H
> > 
> > Please try to avoid adding headers like this if you can at all.
> > 
> > I might ask you to merge the header file in one merge window
> > otherwise and submit the platform code one kernel later, as they
> > tendn to cause us needless dependencies otherwise.
> > 
> 
> Sorry. In v1, Rob suggested putting binding doc and header files in
> a separate patch. The clock driver indeed depends on the header.
> 
> I will put the header and the clock driver in a patch, and keep the
> binding doc in another patch.

Having split patches is better, I was really commenting on the fact
that ideally you would not have a header file at all. If we merge
the header through arm-soc, then you won't be able to merge the
clk driver easily, and if you merge the header through the clk
maintainer, I'm can't take your dts files.

> >> +/* fixed rate */
> >> +#define HI3519_FIXED_400M              1
> >> +#define HI3519_FIXED_200M              2
> >> +#define HI3519_FIXED_125M              3
> >> +#define HI3519_FIXED_150M              4
> >> +#define HI3519_FIXED_75M               5
> >> +#define HI3519_FIXED_300M              6
> >> +#define HI3519_FIXED_50M               7
> >> +#define HI3519_FIXED_24M               8
> >> +#define HI3519_FIXED_3M                        9
> >> +
> >> +/* mux clocks */
> >> +#define HI3519_FMC_MUX                 32
> >> +#define HI3519_I2C_MUX                 33
> >> +#define HI3519_UART_MUX                        34
> >> +#define HI3519_SYSAXI_MUX              35
> >> +
> >> +/*fixed factor clocks*/
> >> +#define HI3519_SYSAPB_CLK              64
> >> +
> >> +/* gate clocks */
> >> +#define HI3519_FMC_CLK                 129
> >> +#define HI3519_UART0_CLK               153
> >> +#define HI3519_UART1_CLK               154
> >> +#define HI3519_UART2_CLK               155
> >> +#define HI3519_UART3_CLK               156
> >> +#define HI3519_UART4_CLK               157
> > 
> > Where do those numbers come from? They are not consecutive, so it sounds
> > like they are directly from the data sheet and won't be needed in the driver.
> > If that's true, just use the numbers directly, as you do for everything
> > else.
> 
> The numbers are defined by myself, not directly from the data sheet. Some numbers
> are reserved for device nodes which will be added later. So they are not consecutive now.

I don't understand. How do you decide which numbers to reserve? If the
numbers are completely arbitrary and you have no idea what other clocks
there are, you can simply have consecutive numbers here and do the driver
accordingly.

If the numbers actually have a real meaning, then you either don't need them
at all, or you could just put all numbers in there that you would eventually need.

> >> +#define HI3519_NR_CLKS                 256
> >> +#define HI3519_NR_RSTS                 256
> >>
> > These seem to not be needed at all.
> 
> These are used in drivers/clk/hisilicon/clk-hi3519.c.

Then move them there. Anything that is not needed by *both* the driver and 
the dts files doesn't belong in here.

	Arnd

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

On 2015/12/4 18:56, Arnd Bergmann wrote:
> On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
>> Hi Arnd,
>>
>> On 2015/12/3 17:44, Arnd Bergmann wrote:
>>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>>>> +#ifndef __DTS_HI3519_CLOCK_H
>>>> +#define __DTS_HI3519_CLOCK_H
>>>
>>> Please try to avoid adding headers like this if you can at all.
>>>
>>> I might ask you to merge the header file in one merge window
>>> otherwise and submit the platform code one kernel later, as they
>>> tendn to cause us needless dependencies otherwise.
>>>
>>
>> Sorry. In v1, Rob suggested putting binding doc and header files in
>> a separate patch. The clock driver indeed depends on the header.
>>
>> I will put the header and the clock driver in a patch, and keep the
>> binding doc in another patch.
> 
> Having split patches is better, I was really commenting on the fact
> that ideally you would not have a header file at all. If we merge
> the header through arm-soc, then you won't be able to merge the
> clk driver easily, and if you merge the header through the clk
> maintainer, I'm can't take your dts files.

Thank you for your comments. Because the clocks in the crg module have
different types and random layouts. If this header file is removed,
the clock driver and the dts files will get very complicated.

Could you help me acknowledge it if I put the header file and clock driver
in a patch?

Could you give me some suggestions If I want to keep this header file?

> 
>>>> +/* fixed rate */
>>>> +#define HI3519_FIXED_400M              1
>>>> +#define HI3519_FIXED_200M              2
>>>> +#define HI3519_FIXED_125M              3
>>>> +#define HI3519_FIXED_150M              4
>>>> +#define HI3519_FIXED_75M               5
>>>> +#define HI3519_FIXED_300M              6
>>>> +#define HI3519_FIXED_50M               7
>>>> +#define HI3519_FIXED_24M               8
>>>> +#define HI3519_FIXED_3M                        9
>>>> +
>>>> +/* mux clocks */
>>>> +#define HI3519_FMC_MUX                 32
>>>> +#define HI3519_I2C_MUX                 33
>>>> +#define HI3519_UART_MUX                        34
>>>> +#define HI3519_SYSAXI_MUX              35
>>>> +
>>>> +/*fixed factor clocks*/
>>>> +#define HI3519_SYSAPB_CLK              64
>>>> +
>>>> +/* gate clocks */
>>>> +#define HI3519_FMC_CLK                 129
>>>> +#define HI3519_UART0_CLK               153
>>>> +#define HI3519_UART1_CLK               154
>>>> +#define HI3519_UART2_CLK               155
>>>> +#define HI3519_UART3_CLK               156
>>>> +#define HI3519_UART4_CLK               157
>>>
>>> Where do those numbers come from? They are not consecutive, so it sounds
>>> like they are directly from the data sheet and won't be needed in the driver.
>>> If that's true, just use the numbers directly, as you do for everything
>>> else.
>>
>> The numbers are defined by myself, not directly from the data sheet. Some numbers
>> are reserved for device nodes which will be added later. So they are not consecutive now.
> 
> I don't understand. How do you decide which numbers to reserve? If the
> numbers are completely arbitrary and you have no idea what other clocks
> there are, you can simply have consecutive numbers here and do the driver
> accordingly.

The clocks are divided into several groups according to their types. The clocks in
a group are expected to have consecutive numbers. So some numbers are reserved for
every group in this file, just like in some existing headers. Other clocks will be
added when other peripherals drivers are submitted. They will use the reserve numbers
and be inserted into existing groups.

Of course it is not required to reserve numbers for later added clocks.

> 
> If the numbers actually have a real meaning, then you either don't need them
> at all, or you could just put all numbers in there that you would eventually need.

The numbers have no hardware meaning actually.

> 
>>>> +#define HI3519_NR_CLKS                 256
>>>> +#define HI3519_NR_RSTS                 256
>>>>
>>> These seem to not be needed at all.
>>
>> These are used in drivers/clk/hisilicon/clk-hi3519.c.
> 
> Then move them there. Anything that is not needed by *both* the driver and 
> the dts files doesn't belong in here.


OK. I will move them into the driver code.

> 
> 	Arnd
> 
> .
> 

Many thanks.

Jiancheng

.

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[Arnd Bergmann]

On Monday 07 December 2015 16:01:03 xuejiancheng wrote:
> On 2015/12/4 18:56, Arnd Bergmann wrote:
> > On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
> >> Hi Arnd,
> >>
> >> On 2015/12/3 17:44, Arnd Bergmann wrote:
> >>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
> >>>> +#ifndef __DTS_HI3519_CLOCK_H
> >>>> +#define __DTS_HI3519_CLOCK_H
> >>>
> >>> Please try to avoid adding headers like this if you can at all.
> >>>
> >>> I might ask you to merge the header file in one merge window
> >>> otherwise and submit the platform code one kernel later, as they
> >>> tendn to cause us needless dependencies otherwise.
> >>>
> >>
> >> Sorry. In v1, Rob suggested putting binding doc and header files in
> >> a separate patch. The clock driver indeed depends on the header.
> >>
> >> I will put the header and the clock driver in a patch, and keep the
> >> binding doc in another patch.
> > 
> > Having split patches is better, I was really commenting on the fact
> > that ideally you would not have a header file at all. If we merge
> > the header through arm-soc, then you won't be able to merge the
> > clk driver easily, and if you merge the header through the clk
> > maintainer, I'm can't take your dts files.
> 
> Thank you for your comments. Because the clocks in the crg module have
> different types and random layouts. If this header file is removed,
> the clock driver and the dts files will get very complicated.
> 
> Could you help me acknowledge it if I put the header file and clock driver
> in a patch?
> 
> Could you give me some suggestions If I want to keep this header file?

If this is another clock controller that has a random register layout,
then adding the header file is the least problematic solution indeed.

> >>> Where do those numbers come from? They are not consecutive, so it sounds
> >>> like they are directly from the data sheet and won't be needed in the driver.
> >>> If that's true, just use the numbers directly, as you do for everything
> >>> else.
> >>
> >> The numbers are defined by myself, not directly from the data sheet. Some numbers
> >> are reserved for device nodes which will be added later. So they are not consecutive now.
> > 
> > I don't understand. How do you decide which numbers to reserve? If the
> > numbers are completely arbitrary and you have no idea what other clocks
> > there are, you can simply have consecutive numbers here and do the driver
> > accordingly.
> 
> The clocks are divided into several groups according to their types. The clocks in
> a group are expected to have consecutive numbers. So some numbers are reserved for
> every group in this file, just like in some existing headers. Other clocks will be
> added when other peripherals drivers are submitted. They will use the reserve numbers
> and be inserted into existing groups.
> 
> Of course it is not required to reserve numbers for later added clocks.

Are you able to enumerate all the clocks then? If all clocks that are
supported by this clock controllers have names in the data sheet, I
would prefer to just list them all now rather than only enter the ones
we already need, to avoid having future merge conflicts.

Then we just need to add code to support those clocks when we need them,
but that can be done in parallel to adding the DT nodes and the driver,
rather than strongly serializing the patch flow on the header file patches.

	Arnd

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

On 2015/12/7 17:36, Arnd Bergmann wrote:
> On Monday 07 December 2015 16:01:03 xuejiancheng wrote:
>> On 2015/12/4 18:56, Arnd Bergmann wrote:
>>> On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
>>>> Hi Arnd,
>>>>
>>>> On 2015/12/3 17:44, Arnd Bergmann wrote:
>>>>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>>>>>> +#ifndef __DTS_HI3519_CLOCK_H
>>>>>> +#define __DTS_HI3519_CLOCK_H
>>>>>
>>>>> Please try to avoid adding headers like this if you can at all.
>>>>>
>>>>> I might ask you to merge the header file in one merge window
>>>>> otherwise and submit the platform code one kernel later, as they
>>>>> tendn to cause us needless dependencies otherwise.
>>>>>
>>>>
>>>> Sorry. In v1, Rob suggested putting binding doc and header files in
>>>> a separate patch. The clock driver indeed depends on the header.
>>>>
>>>> I will put the header and the clock driver in a patch, and keep the
>>>> binding doc in another patch.
>>>
>>> Having split patches is better, I was really commenting on the fact
>>> that ideally you would not have a header file at all. If we merge
>>> the header through arm-soc, then you won't be able to merge the
>>> clk driver easily, and if you merge the header through the clk
>>> maintainer, I'm can't take your dts files.
>>
>> Thank you for your comments. Because the clocks in the crg module have
>> different types and random layouts. If this header file is removed,
>> the clock driver and the dts files will get very complicated.
>>
>> Could you help me acknowledge it if I put the header file and clock driver
>> in a patch?
>>
>> Could you give me some suggestions If I want to keep this header file?
> 
> If this is another clock controller that has a random register layout,
> then adding the header file is the least problematic solution indeed.
> 
>>>>> Where do those numbers come from? They are not consecutive, so it sounds
>>>>> like they are directly from the data sheet and won't be needed in the driver.
>>>>> If that's true, just use the numbers directly, as you do for everything
>>>>> else.
>>>>
>>>> The numbers are defined by myself, not directly from the data sheet. Some numbers
>>>> are reserved for device nodes which will be added later. So they are not consecutive now.
>>>
>>> I don't understand. How do you decide which numbers to reserve? If the
>>> numbers are completely arbitrary and you have no idea what other clocks
>>> there are, you can simply have consecutive numbers here and do the driver
>>> accordingly.
>>
>> The clocks are divided into several groups according to their types. The clocks in
>> a group are expected to have consecutive numbers. So some numbers are reserved for
>> every group in this file, just like in some existing headers. Other clocks will be
>> added when other peripherals drivers are submitted. They will use the reserve numbers
>> and be inserted into existing groups.
>>
>> Of course it is not required to reserve numbers for later added clocks.
> 
> Are you able to enumerate all the clocks then? If all clocks that are
> supported by this clock controllers have names in the data sheet, I
> would prefer to just list them all now rather than only enter the ones
> we already need, to avoid having future merge conflicts.
> 
> Then we just need to add code to support those clocks when we need them,
> but that can be done in parallel to adding the DT nodes and the driver,
> rather than strongly serializing the patch flow on the header file patches.

OK. I'll enumerate all the clocks in the data sheet in next version patch.

Thank you.

> 
> 	Arnd
> 
> .
> 

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

On 2015/12/7 17:36, Arnd Bergmann wrote:
> On Monday 07 December 2015 16:01:03 xuejiancheng wrote:
>> On 2015/12/4 18:56, Arnd Bergmann wrote:
>>> On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
>>>> Hi Arnd,
>>>>
>>>> On 2015/12/3 17:44, Arnd Bergmann wrote:
>>>>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>>>>>> +#ifndef __DTS_HI3519_CLOCK_H
>>>>>> +#define __DTS_HI3519_CLOCK_H
>>>>>
>>>>> Please try to avoid adding headers like this if you can at all.
>>>>>
>>>>> I might ask you to merge the header file in one merge window
>>>>> otherwise and submit the platform code one kernel later, as they
>>>>> tendn to cause us needless dependencies otherwise.
>>>>>
>>>>
>>>> Sorry. In v1, Rob suggested putting binding doc and header files in
>>>> a separate patch. The clock driver indeed depends on the header.
>>>>
>>>> I will put the header and the clock driver in a patch, and keep the
>>>> binding doc in another patch.
>>>
>>> Having split patches is better, I was really commenting on the fact
>>> that ideally you would not have a header file at all. If we merge
>>> the header through arm-soc, then you won't be able to merge the
>>> clk driver easily, and if you merge the header through the clk
>>> maintainer, I'm can't take your dts files.
>>
>> Thank you for your comments. Because the clocks in the crg module have
>> different types and random layouts. If this header file is removed,
>> the clock driver and the dts files will get very complicated.
>>
>> Could you help me acknowledge it if I put the header file and clock driver
>> in a patch?
>>
>> Could you give me some suggestions If I want to keep this header file?
> 
> If this is another clock controller that has a random register layout,
> then adding the header file is the least problematic solution indeed.

Is it OK if I put the header file and the clock driver in a patch?

If it's not OK, could you tell me how should I separate the patches?

Thank you.

> 
>>>>> Where do those numbers come from? They are not consecutive, so it sounds
>>>>> like they are directly from the data sheet and won't be needed in the driver.
>>>>> If that's true, just use the numbers directly, as you do for everything
>>>>> else.
>>>>
>>>> The numbers are defined by myself, not directly from the data sheet. Some numbers
>>>> are reserved for device nodes which will be added later. So they are not consecutive now.
>>>
>>> I don't understand. How do you decide which numbers to reserve? If the
>>> numbers are completely arbitrary and you have no idea what other clocks
>>> there are, you can simply have consecutive numbers here and do the driver
>>> accordingly.
>>
>> The clocks are divided into several groups according to their types. The clocks in
>> a group are expected to have consecutive numbers. So some numbers are reserved for
>> every group in this file, just like in some existing headers. Other clocks will be
>> added when other peripherals drivers are submitted. They will use the reserve numbers
>> and be inserted into existing groups.
>>
>> Of course it is not required to reserve numbers for later added clocks.
> 
> Are you able to enumerate all the clocks then? If all clocks that are
> supported by this clock controllers have names in the data sheet, I
> would prefer to just list them all now rather than only enter the ones
> we already need, to avoid having future merge conflicts.
> 
> Then we just need to add code to support those clocks when we need them,
> but that can be done in parallel to adding the DT nodes and the driver,
> rather than strongly serializing the patch flow on the header file patches.
> 
> 	Arnd
> 
> .
> 

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[Arnd Bergmann]

On Tuesday 08 December 2015 17:45:25 xuejiancheng wrote:
> On 2015/12/7 17:36, Arnd Bergmann wrote:
> > On Monday 07 December 2015 16:01:03 xuejiancheng wrote:
> >> On 2015/12/4 18:56, Arnd Bergmann wrote:
> >>> On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
> >>>> Hi Arnd,
> >>>>
> >>>> On 2015/12/3 17:44, Arnd Bergmann wrote:
> >>>>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
> >>>>>> +#ifndef __DTS_HI3519_CLOCK_H
> >>>>>> +#define __DTS_HI3519_CLOCK_H
> >>>>>
> >>>>> Please try to avoid adding headers like this if you can at all.
> >>>>>
> >>>>> I might ask you to merge the header file in one merge window
> >>>>> otherwise and submit the platform code one kernel later, as they
> >>>>> tendn to cause us needless dependencies otherwise.
> >>>>>
> >>>>
> >>>> Sorry. In v1, Rob suggested putting binding doc and header files in
> >>>> a separate patch. The clock driver indeed depends on the header.
> >>>>
> >>>> I will put the header and the clock driver in a patch, and keep the
> >>>> binding doc in another patch.
> >>>
> >>> Having split patches is better, I was really commenting on the fact
> >>> that ideally you would not have a header file at all. If we merge
> >>> the header through arm-soc, then you won't be able to merge the
> >>> clk driver easily, and if you merge the header through the clk
> >>> maintainer, I'm can't take your dts files.
> >>
> >> Thank you for your comments. Because the clocks in the crg module have
> >> different types and random layouts. If this header file is removed,
> >> the clock driver and the dts files will get very complicated.
> >>
> >> Could you help me acknowledge it if I put the header file and clock driver
> >> in a patch?
> >>
> >> Could you give me some suggestions If I want to keep this header file?
> > 
> > If this is another clock controller that has a random register layout,
> > then adding the header file is the least problematic solution indeed.
> 
> Is it OK if I put the header file and the clock driver in a patch?
> 
> If it's not OK, could you tell me how should I separate the patches?

It's ok to do it like this, but then I can't easily merge any DT changes
based on the header file into the arm-soc tree in the same merge window.
Staging out the .dts files by one merge window is the easiest solution
here, otherwise you need to set up a shared branch with the headers
changes and base both the clk driver and the dts branch on top of that
and cannot rebase those patches.

	Arnd