Re: [PATCH v4 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Mike，
   I am sorry. I have to correct my answer about using CLK_OF_DECLARE.  

On 2016/1/5 15:21, xuejiancheng wrote:
> Hi Mike,
>Happy new year to you!
>Thank you for taking time to reply.
> 
> On 2015/12/31 8:23, Michael Turquette wrote:
>> Hello Jiancheng Xue,
>>
>> Quoting Jiancheng Xue (2015-12-29 17:43:47)
>>> The CRG(Clock and Reset Generator) block provides clock
>>> and reset signals for other modules in hi3519 soc.
>>>
>>> Signed-off-by: Jiancheng Xue 
>>> ---
>>>  .../devicetree/bindings/clock/hi3519-crg.txt   |  46 +++
>>>  drivers/clk/hisilicon/Kconfig  |   7 +
>>>  drivers/clk/hisilicon/Makefile |   2 +
>>>  drivers/clk/hisilicon/clk-hi3519.c | 103 ++
>>>  drivers/clk/hisilicon/reset.c  | 149 
>>> +
>>>  drivers/clk/hisilicon/reset.h  |  32 +
>>>  include/dt-bindings/clock/hi3519-clock.h   |  43 ++
>>>  7 files changed, 382 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>>  create mode 100644 include/dt-bindings/clock/hi3519-clock.h
>>
>> Please keep Philipp Zabel Cc'd for reset-related patches. I've added
>> him to Cc.
>>
> OK.
> 
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>> new file mode 100644
>>> index 000..2d23950
>>> --- /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 .
>>> +
>>> +- #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@1201 {
>>> +   compatible = "hisilicon,hi3519-crg";
>>> +reg = <0x1201 0x1>;
>>> +#clock-cells = <1>;
>>> +#reset-cells = <2>;
>>> +};
>>> +
>>> +Example: consumer nodes
>>> +i2c0: i2c@1211 {
>>> +   compatible = "hisilicon,hi3519-i2c";
>>> +reg = <0x1211 0x1000>;
>>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>>> +resets = <&CRG 0xe4 0>;
>>> +};
>>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>>> index e434854..b6baebf 100644
>>> --- a/drivers/clk/hisilicon/Kconfig
>>> +++ b/drivers/clk/hisilicon/Kconfig
>>> @@ -1,3 +1,10 @@
>>> +config COMMON_CLK_HI3519
>>> +   tristate "Clock Driver for Hi3519"
>>> +   depends on ARCH_HISI
>>> +   default y
>>> +   help
>>> + Build the clock driver for hi3519.
>>> +
>>>  config COMMON_CLK_HI6220
>>> bool "Hi6220 Clock Driver"
>>> depends on ARCH_HISI || COMPILE_TEST
>>> diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
>>

Re: [PATCH v4 5/6] mfd: dt-bindings: add device tree bindings for Hi3519 sysctrl

[xuejiancheng]

On 2016/1/5 18:12, Philipp Zabel wrote:
> Am Mittwoch, den 30.12.2015, 09:43 +0800 schrieb Jiancheng Xue:
>> Add device tree bindings for Hi3519 system controller.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  Documentation/devicetree/bindings/mfd/hi3519.txt | 14 ++
>>  1 file changed, 14 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/hi3519.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/hi3519.txt 
>> b/Documentation/devicetree/bindings/mfd/hi3519.txt
>> new file mode 100644
>> index 000..2536edc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/hi3519.txt
>> @@ -0,0 +1,14 @@
>> +* Hisilicon Hi3519 System Controller Block
>> +
>> +This bindings use the following binding:
>> +Dcumentation/devicetree/bindings/clock/clock-bindings.txt
> 
> Typo: "Documentation"
> - but I don't see the clock bindings being used here at all.
> Maybe just drop this sentence?
> 

Sorry about this mistake. Thank you very much.

>> +
>> +Required properties:
>> +- compatible: "hisilicon,hi3519-sysctrl".
>> +- reg: the register region of this block
>> +
>> +Examples:
>> +sysctrl: system-controller@1201 {
>> +compatible = "hisilicon,hi3519-sysctrl", "syscon";
>> +reg = <0x1201 0x1000>;
>> +};
> 
> regards
> Philipp
> 
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Philipp,
   Thank you very much for your quick reply.

On 2016/1/5 18:12, Philipp Zabel wrote:
> H Jiancheng,
> 
> Am Mittwoch, den 30.12.2015, 09:43 +0800 schrieb Jiancheng Xue:
>> The CRG(Clock and Reset Generator) block provides clock
>> and reset signals for other modules in hi3519 soc.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-crg.txt   |  46 +++
>>  drivers/clk/hisilicon/Kconfig  |   7 +
>>  drivers/clk/hisilicon/Makefile |   2 +
>>  drivers/clk/hisilicon/clk-hi3519.c | 103 ++
>>  drivers/clk/hisilicon/reset.c  | 149 
>> +
>>  drivers/clk/hisilicon/reset.h  |  32 +
>>  include/dt-bindings/clock/hi3519-clock.h   |  43 ++
>>  7 files changed, 382 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>  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 000..2d23950
>> --- /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 .
>> +
>> +- #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.
> 
> Are the resets controlled by single bits spread around the register
> space? If so, I'm fine with this binding.
> 

Yes, you are right.

>> +Example: CRG nodes
>> +CRG: clock-reset-controller@1201 {
>> +compatible = "hisilicon,hi3519-crg";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@1211 {
>> +compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xe4 0>;
>> +};
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> index e434854..b6baebf 100644
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -1,3 +1,10 @@
>> +config COMMON_CLK_HI3519
>> +tristate "Clock Driver for Hi3519"
>> +depends on ARCH_HISI
>> +default y
>> +help
>> +  Build the clock driver for hi3519.
>> +
>>  config COMMON_CLK_HI6220
>>  bool "Hi6220 Clock Driver"
>>  depends on ARCH_HISI || COMPILE_TEST
>> diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
>> index 74dba31..3f57b09 100644
>> --- a/drivers/clk/hisilicon/Makefile
>> +++ b/drivers/clk/hisilicon/Makefile
>> @@ -4,8 +4,10 @@
>>  
>>  obj-y   += clk.o clkgate-separated.o clkdivider-hi6220.o
>>  
>> +obj-$(CONFIG_RESET_CONTROLLER)  += reset.o
>>  obj-$(CONFIG_ARCH_HI3xxx)   += clk-hi3620.o
>>  obj-$(CONFIG_ARCH_HIP04)+= clk-hip04.o
>>  obj-$(CONFIG_ARCH_HIX5HD2)  += clk-hix5hd2.o
>>  obj-$(CONFIG_COMMON_CLK_HI6220) += clk-hi6220.o
>>  obj-$(CONFIG_STUB_CLK_HI6220)   += clk-hi6220-stub.o
>> +obj-$(CONFIG_COMMON_CLK_HI3519) += clk-hi3519.o
>> diff --git a/drivers/clk/hisilicon/clk-hi3519.c 
>> b/drivers/clk/hisilicon/clk-hi3519.c
>> new file mode 100644
>> index 000..e220234
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-hi3519.c
>> @@ -0,0 +1,103 @@
>> +/*
>> + * 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 WITHO

Re: [PATCH v4 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Mike,
   Happy new year to you!
   Thank you for taking time to reply.

On 2015/12/31 8:23, Michael Turquette wrote:
> Hello Jiancheng Xue,
> 
> Quoting Jiancheng Xue (2015-12-29 17:43:47)
>> The CRG(Clock and Reset Generator) block provides clock
>> and reset signals for other modules in hi3519 soc.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-crg.txt   |  46 +++
>>  drivers/clk/hisilicon/Kconfig  |   7 +
>>  drivers/clk/hisilicon/Makefile |   2 +
>>  drivers/clk/hisilicon/clk-hi3519.c | 103 ++
>>  drivers/clk/hisilicon/reset.c  | 149 
>> +
>>  drivers/clk/hisilicon/reset.h  |  32 +
>>  include/dt-bindings/clock/hi3519-clock.h   |  43 ++
>>  7 files changed, 382 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>  create mode 100644 include/dt-bindings/clock/hi3519-clock.h
> 
> Please keep Philipp Zabel Cc'd for reset-related patches. I've added
> him to Cc.
> 
OK.

>>
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> new file mode 100644
>> index 000..2d23950
>> --- /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 .
>> +
>> +- #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@1201 {
>> +   compatible = "hisilicon,hi3519-crg";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@1211 {
>> +   compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xe4 0>;
>> +};
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> index e434854..b6baebf 100644
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -1,3 +1,10 @@
>> +config COMMON_CLK_HI3519
>> +   tristate "Clock Driver for Hi3519"
>> +   depends on ARCH_HISI
>> +   default y
>> +   help
>> + Build the clock driver for hi3519.
>> +
>>  config COMMON_CLK_HI6220
>> bool "Hi6220 Clock Driver"
>> depends on ARCH_HISI || COMPILE_TEST
>> diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
>> index 74dba31..3f57b09 100644
>> --- a/drivers/clk/hisilicon/Makefile
>> +++ b/drivers/clk/hisilicon/Makefile
>> @@ -4,8 +4,10 @@
>>  
>>  obj-y  += clk.o clkgate-separated.o clkdivider-hi6220.o
>>  
>> +obj-$(CONFIG_RESET_CONTROLLER) += reset.o
>>  obj-$(CONFIG_ARCH_HI3xxx)  += clk-hi3620.o
>>  obj-$(CONFIG_ARCH_HIP04)   += clk-hip04.o
>>  obj-$(CONFIG_ARCH_HIX5HD2) += clk-hix5hd2.o
>>  obj-$(CONFIG_COMMON_CLK_HI6220)+= clk-hi6220.o
>>  obj-$(CONFIG_STUB_CLK_HI6220)  += clk-hi6220-stub.o
>> +obj-$(CONFIG_COMMON_CLK_HI3519)+= clk-hi3519.o
>> diff --git a/drivers/clk/hisilicon/clk-hi3519.c 
>> b/drivers/clk/hisilicon/clk-hi3519.c
>> new file mode 100644
>> index 000..e220234
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-hi3519.c
>> @@ -0,0 +1,103 @@
>> +/*
>> + * 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 AN

Re: [PATCH] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[xuejiancheng]

Hi Rob,
   Happy new year to you! Thank you for your patience.

On 2016/1/1 6:26, Rob Herring wrote:
> On Wed, Dec 30, 2015 at 10:26:11AM +0800, Jiancheng Xue wrote:
>> add hisilicon spi-nor flash controller driver
>>
>> Signed-off-by: Binquan Peng 
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/spi/spi-hisi-sfc.txt   |  24 +
>>  drivers/mtd/spi-nor/Kconfig|   7 +
>>  drivers/mtd/spi-nor/Makefile   |   1 +
>>  drivers/mtd/spi-nor/hisi-sfc.c | 505 
>> +
>>  4 files changed, 537 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt
>>  create mode 100644 drivers/mtd/spi-nor/hisi-sfc.c
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt 
>> b/Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt
>> new file mode 100644
>> index 000..170885a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt
>> @@ -0,0 +1,24 @@
>> +HiSilicon SPI-NOR Flash Controller
>> +
>> +Required properties:
>> +- compatible : Should be "hisilicon,hisi-sfc".
>> +- address-cells: number of cells required to define a chip select
>> +address on the SPI bus. Should be set to 1. See spi-bus.txt.
>> +- size-cells:Should be 0.
> 
> How about some consistency in the spacing around the ':'.

OK. I'll correct it in next version. Thank you.

> 
>> +- reg : Offset and length of the register set for the controller device.
>> +- reg-names: Must include the following two entries:"control","memory".
>^ ^
> Spaces needed.

OK

> 
>> +- clocks: handle to spi-nor flash controller clock.
>> +
>> +Example:
>> +spi-nor-controller@1400 {
>> +compatible = "hisilicon,hisi-sfc";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x1000 0x1000>, <0x1400 0x100>;
>> +reg-names = "control", "memory";
>> +clocks = <&clock HI3519_FMC_CLK>;
>> +spi-nor@0 {
>> +compatible = "jedec,spi-nor";
>> +reg = <0>;
>> +};
>> +};
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 2fe2a7e..7fe1564 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -30,6 +30,12 @@ config SPI_FSL_QUADSPI
>>This controller does not support generic SPI. It only supports
>>SPI NOR.
>>  
>> +config SPI_HISI_SFC
>> +tristate "Hisilicon SPI-NOR Flash Controller(SFC)"
>> +depends on ARCH_HISI
>> +help
>> +  This enables support for hisilicon SPI-NOR flash controller.
>> +
>>  config SPI_NXP_SPIFI
>>  tristate "NXP SPI Flash Interface (SPIFI)"
>>  depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
>> @@ -41,4 +47,5 @@ config SPI_NXP_SPIFI
>>Flash. Enable this option if you have a device with a SPIFI
>>controller and want to access the Flash as a mtd device.
>>  
>> +
> 
> Drop this spurious change.

OK. Thank you. I'll pay more attention to this later.

> 
>>  endif # MTD_SPI_NOR
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index e5e..8cea3c5 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,3 +1,4 @@
>>  obj-$(CONFIG_MTD_SPI_NOR)   += spi-nor.o
>>  obj-$(CONFIG_SPI_FSL_QUADSPI)   += fsl-quadspi.o
>> +obj-$(CONFIG_SPI_HISI_SFC)  += hisi-sfc.o
>>  obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o
>> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
>> new file mode 100644
>> index 000..fd9649a
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
>> @@ -0,0 +1,505 @@
>> +/* HiSilicon SPI Nor Flash Controller Driver
>> + *
>> + * 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 .
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> Some say to alphabetize includes. I don't care so much, but at least 
> group subdirectories.

Sorry about that. I'll reorder them.

> 
> Rob
> 
> .
> 

Jiancheng

.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] ARM: dts: add dts files for Hi3519

[xuejiancheng]

Hi Rob,

On 2015/12/12 0:28, Rob Herring wrote:
> On Fri, Dec 11, 2015 at 03:45:21PM +0800, Jiancheng Xue wrote:
>> add dts files for Hi3519
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  arch/arm/boot/dts/Makefile|   2 +
>>  arch/arm/boot/dts/hi3519-demb.dts |  42 +++
>>  arch/arm/boot/dts/hi3519.dtsi | 142 
>> ++
>>  3 files changed, 186 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/hi3519-demb.dts
>>  create mode 100644 arch/arm/boot/dts/hi3519.dtsi
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 30bbc37..1ff3ed9 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -135,6 +135,8 @@ dtb-$(CONFIG_ARCH_EXYNOS5) += \
>>  exynos5800-peach-pi.dtb
>>  dtb-$(CONFIG_ARCH_HI3xxx) += \
>>  hi3620-hi4511.dtb
>> +dtb-$(CONFIG_ARCH_HISI) += \
>> +hi3519-demb.dtb
>>  dtb-$(CONFIG_ARCH_HIX5HD2) += \
>>  hisi-x5hd2-dkb.dtb
>>  dtb-$(CONFIG_ARCH_HIGHBANK) += \
>> diff --git a/arch/arm/boot/dts/hi3519-demb.dts 
>> b/arch/arm/boot/dts/hi3519-demb.dts
>> new file mode 100644
>> index 000..6991ab6
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hi3519-demb.dts
>> @@ -0,0 +1,42 @@
>> +/*
>> + * 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 .
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +#include "hi3519.dtsi"
>> +
>> +/ {
>> +model = "HiSilicon HI3519 DEMO Board";
>> +compatible = "hisilicon,hi3519";
>> +
>> +aliases {
>> +serial0 = &uart0;
>> +};
>> +
>> +memory {
>> +device_type = "memory";
>> +reg = <0x8000 0x4000>;
>> +};
>> +};
>> +
>> +&uart0 {
>> +status = "okay";
>> +};
>> +
>> +&dual_timer0 {
>> +status = "okay";
>> +};
>> diff --git a/arch/arm/boot/dts/hi3519.dtsi b/arch/arm/boot/dts/hi3519.dtsi
>> new file mode 100644
>> index 000..50b736e
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hi3519.dtsi
>> @@ -0,0 +1,142 @@
>> +/*
>> + * 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 .
>> + *
>> + */
>> +
>> +#include "skeleton.dtsi"
> 
> Don't include skeleton.dtsi. We've decided it was a bad idea.

OK.

> 
>> +#include 
>> +/ {
>> +cpus {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +cpu@0 {
> 
> Single core system? Add the other cpu nodes if not. Adding them doesn't 
> have to be in sync with SMP kernel support.

Hi3519 includes an A7 core and an A17 core. But it will run in AMP mode.
The A17 core is reserved for customers for special use. We can treat the
system as a single core system here.

> 
>> +device_type = "cpu";
>> +compatible = "arm,cortex-a7";
>> +reg = <0>;
>> +};
>> +};
>> +
>> +gic: interrupt-controller@1030 {
>> +compatible = "arm,cortex-a7-gic";
>> +#interrupt-cells = <3>;
>> +interrupt-controller;
>> +reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
>> +};
>> +
>> +soc {
>> +#address-cells = <1>;
>> +#size-cells = <1>;
>> +compatible = "simple-bus";
>> +interrupt-parent = <&gic>;
>> +ranges;
> 
> Looks like everything is in 0x12xx range, so you should add actual 
> translation here if that's the case.

Other device nodes will be added later which are not in 0x12xx range.

> 
>> +
>> +amba {
> 
> Is this actually a separate bus in the physical design of the chip?
> 
>> +#address-cells = <1>;
>> +

Re: [PATCH v3 3/7] ARM: hisi: add dt_machine definition for Hi3519

[xuejiancheng]

On 2015/12/11 23:21, Rob Herring wrote:
> On Fri, Dec 11, 2015 at 03:45:17PM +0800, Jiancheng Xue wrote:
>> add dt_machine definition for hi3519.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  arch/arm/mach-hisi/hisilicon.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm/mach-hisi/hisilicon.c b/arch/arm/mach-hisi/hisilicon.c
>> index 8cc6215..010d8a2 100644
>> --- a/arch/arm/mach-hisi/hisilicon.c
>> +++ b/arch/arm/mach-hisi/hisilicon.c
>> @@ -81,3 +81,12 @@ static const char *const hip01_compat[] __initconst = {
>>  DT_MACHINE_START(HIP01, "Hisilicon HIP01 (Flattened Device Tree)")
>>  .dt_compat  = hip01_compat,
>>  MACHINE_END
>> +
>> +static const char *const hi3519_compat[] __initconst = {
>> +"hisilicon,hi3519",
>> +NULL,
>> +};
> 
> You should just have 1 mach desc with multiple compatible strings to 
> match against, not 1 mach desc per compatible string.

Yes, you're right. But Hi3519 is a soc in a new family. It doesn't belong to
any other existing mach descs. And more hi3519 compatible boards will be added
in this mach desc afterwards.
Can I do it like this now, and combine them at a proper time?

Or just add a more generic mach desc, then other socs like hix5hd2/hip01/hip04 
can use this?
static const char *const hisilicon_compat[] __initconst = {
"hisilicon,hi3519",
NULL,
};

DT_MACHINE_START(HISILICON_DT, "Hisilicon")
.dt_compat  = hisilicon_compat,
MACHINE_END



> Rob
> 
>> +
>> +DT_MACHINE_START(HI3519_DT, "Hisilicon Hi3519")
>> +.dt_compat  = hi3519_compat,
>> +MACHINE_END
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/7] clk: hisilicon: add dt-binding document for Hi3519 CRG

[xuejiancheng]

On 2015/12/11 23:19, Rob Herring wrote:
> On Fri, Dec 11, 2015 at 03:45:16PM +0800, Jiancheng Xue wrote:
>> add dt-binding document for Hi3519 CRG block
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-crg.txt   | 46 
>> ++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> new file mode 100644
>> index 000..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 .
> 
> This header should be part of this patch.

Because the header is also depended on by the clock driver. It's also a problem
if I separate it from the clock driver patch.

Is it OK if I put this binding file into the clock driver patch?
> 
>> +
>> +- #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 {
> 
> clock-reset-controller@1201

OK.

> 
>> +compatible = "hisilicon,hi3519-crg";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@0x1211 {
> 
> Drop '0x'

OK.

> 
>> +compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xE4 0>;
> 
> lowercase hex please

OK.

> 
>> +};
>> -- 
>> 1.9.1
>>
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/10 16:04, Arnd Bergmann wrote:
> On Thursday 10 December 2015 14:32:05 xuejiancheng wrote:
>>>
>>> This is not what I meant. You have to use "syscon" as the most generic
>>> "compatible" value here, but should add a machine specific string
>>> as a more specific one. "hisilicon,sysctrl" is not appropriate because
>>> it does not identify the IP block uniquely, you can only use that
>>> in combination with another more specific string.
>>
>> OK. I will use "hisilicon,hi3519-syscon" and "syscon" as the compatible value
>> for the sysctrl node in hi3519.dtsi.
>>
> 
> Why hisilicon,hi3519-syscon instead of hisilicon,hi3519-sysctrl?

> Is this not called a sysctrl at all in the datasheet then?

It is OK to use hisilicon,hi3519-sysctrl.

I thought syscon was more commonly used as abbr. of system-controller in the 
kernel code.

>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/9 23:31, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 11:54:51 xuejiancheng wrote:
>> On 2015/12/7 14:37, xuejiancheng wrote:
>>>
>>> On 2015/12/4 18:49, Arnd Bergmann wrote:
>>>> On Friday 04 December 2015 10:27:58 xuejiancheng wrote:
>>>>>>
>>>> Maybe split out the sysctrl binding from
>>>> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt, as it has
>>>> you already have a couple of those, and it's not clear how they relate
>>>> to one another.
>>>>
>>>> If we introduce a string for all hip04 compatible sysctrl devices, we 
>>>> should
>>>> document that and use it consistently, so hi3519 becomes
>>>>
>>>>  compatible = "hisilicon,hi3519-sysctrl", "hisilicon,hip04-sysctrl", 
>>>> "hisilicon,sysctrl";
>>>>
>>>> but I'd clarify in the binding documentation that "hisilicon,sysctrl" 
>>>> should
>>>> only be used for hip04 and hi3519 but not the others.
>>>>
>>>> As this seems to be a standard part, we can also think about making a
>>>> high-level driver for in in drivers/soc rather than relying on the syscon
>>>> driver which we tend to use more for one-off devices with random register
>>>> layouts.
>>>>
>>>Sorry. I didn't understand your meaning well and maybe I gave you a 
>>> wrong description.
>>> Please allow me to clarify it again.
>>>The "sysctrl" nodes here is just used for the "reboot" function. It is 
>>> corresponding to
>>> the driver "drivers/power/reset/hisi-reboot.c". The compatible string in 
>>> the driver is
>>> "hisilicon,sysctrl".
>>>The layout of this block is also different from the one in HiP04.
>>
>> I'll use "syscon" as the compatible value for sysctrl node and 
>> "syscon-reboot" for a new reboot node.
>>
>>
> 
> This is not what I meant. You have to use "syscon" as the most generic
> "compatible" value here, but should add a machine specific string
> as a more specific one. "hisilicon,sysctrl" is not appropriate because
> it does not identify the IP block uniquely, you can only use that
> in combination with another more specific string.

OK. I will use "hisilicon,hi3519-syscon" and "syscon" as the compatible value
for the sysctrl node in hi3519.dtsi.

Thank you!

> 
> That way, we have to option to create a high-level driver for the IP
> block later if it turns out that we need some more generic functionality
> that is provided by those registers.
> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/7 14:37, xuejiancheng wrote:
> 
> On 2015/12/4 18:49, Arnd Bergmann wrote:
>> On Friday 04 December 2015 10:27:58 xuejiancheng wrote:
>>>>
>>>>> +sysctrl: system-controller@1202 {
>>>>> +compatible = "hisilicon,sysctrl";
>>>>> +reg = <0x1202 0x1000>;
>>>>> +reboot-offset = <0x4>;
>>>>> +};
>>>>
>>>> Is this one identical to the one in hip04?
>>>>
>>>> If not, pick a new unique compatible string
>>>
>>> Yes. It's compatible with the one in hip04.
>>
>> Ok, we should add a compatible string for that then, as the hip04 apparently
>> has a slightly different layout from hip01.
>>
>> Can you add a separate patch to clarify the existing hisilicon,sysctrl nodes?
>>
>> It look like hi3620 accidentally uses the same string as hip04 already
>> but is actually a bit different.
>>
>> Maybe split out the sysctrl binding from
>> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt, as it has
>> you already have a couple of those, and it's not clear how they relate
>> to one another.
>>
>> If we introduce a string for all hip04 compatible sysctrl devices, we should
>> document that and use it consistently, so hi3519 becomes
>>
>>  compatible = "hisilicon,hi3519-sysctrl", "hisilicon,hip04-sysctrl", 
>> "hisilicon,sysctrl";
>>
>> but I'd clarify in the binding documentation that "hisilicon,sysctrl" should
>> only be used for hip04 and hi3519 but not the others.
>>
>> As this seems to be a standard part, we can also think about making a
>> high-level driver for in in drivers/soc rather than relying on the syscon
>> driver which we tend to use more for one-off devices with random register
>> layouts.
>>
>Sorry. I didn't understand your meaning well and maybe I gave you a wrong 
> description.
> Please allow me to clarify it again.
>The "sysctrl" nodes here is just used for the "reboot" function. It is 
> corresponding to
> the driver "drivers/power/reset/hisi-reboot.c". The compatible string in the 
> driver is
> "hisilicon,sysctrl".
>The layout of this block is also different from the one in HiP04.

I'll use "syscon" as the compatible value for sysctrl node and "syscon-reboot" 
for a new reboot node.

Thank you.

> 
>>>>> +
>>>>> +crg: crg@1201 {
>>>>> +compatible = "hisilicon,hi3519-crg";
>>>>
>>>>
>>>> what is a "crg"? Is there a standard name for these?
>>>
>>> The "crg" means Clock and Reset Generator. It's a block which supplies clock
>>> and reset signals for other modules in the chip. I used "hi3519-clock"
>>> in last version patch. Rob Herring suggested that it's better to use 
>>> "hi3519-crg"
>>> as the compatible string if it's a whole block.
>>>
>>> what about writing like this？
>>>
>>> crg: clock-reset-controller@1201 {
>>> compatible = "hisilicon,hi3519-crg";
>>> }
>>>
>>
>> Ok, that's better.
>>
>>  Arnd
>>
>> .
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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_3M9
>>>> +
>>>> +/* mux clocks */
>>>> +#define HI3519_FMC_MUX 32
>>>> +#define HI3519_I2C_MUX 33
>>>> +#define HI3519_UART_MUX34
>>>> +#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

.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/4 18:49, Arnd Bergmann wrote:
> On Friday 04 December 2015 10:27:58 xuejiancheng wrote:
>>>
>>>> +sysctrl: system-controller@1202 {
>>>> +compatible = "hisilicon,sysctrl";
>>>> +reg = <0x1202 0x1000>;
>>>> +reboot-offset = <0x4>;
>>>> +};
>>>
>>> Is this one identical to the one in hip04?
>>>
>>> If not, pick a new unique compatible string
>>
>> Yes. It's compatible with the one in hip04.
> 
> Ok, we should add a compatible string for that then, as the hip04 apparently
> has a slightly different layout from hip01.
> 
> Can you add a separate patch to clarify the existing hisilicon,sysctrl nodes?
> 
> It look like hi3620 accidentally uses the same string as hip04 already
> but is actually a bit different.
> 
> Maybe split out the sysctrl binding from
> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt, as it has
> you already have a couple of those, and it's not clear how they relate
> to one another.
> 
> If we introduce a string for all hip04 compatible sysctrl devices, we should
> document that and use it consistently, so hi3519 becomes
> 
>   compatible = "hisilicon,hi3519-sysctrl", "hisilicon,hip04-sysctrl", 
> "hisilicon,sysctrl";
> 
> but I'd clarify in the binding documentation that "hisilicon,sysctrl" should
> only be used for hip04 and hi3519 but not the others.
> 
> As this seems to be a standard part, we can also think about making a
> high-level driver for in in drivers/soc rather than relying on the syscon
> driver which we tend to use more for one-off devices with random register
> layouts.
> 
   Sorry. I didn't understand your meaning well and maybe I gave you a wrong 
description.
Please allow me to clarify it again.
   The "sysctrl" nodes here is just used for the "reboot" function. It is 
corresponding to
the driver "drivers/power/reset/hisi-reboot.c". The compatible string in the 
driver is
"hisilicon,sysctrl".
   The layout of this block is also different from the one in HiP04.

>>>> +
>>>> +crg: crg@1201 {
>>>> +compatible = "hisilicon,hi3519-crg";
>>>
>>>
>>> what is a "crg"? Is there a standard name for these?
>>
>> The "crg" means Clock and Reset Generator. It's a block which supplies clock
>> and reset signals for other modules in the chip. I used "hi3519-clock"
>> in last version patch. Rob Herring suggested that it's better to use 
>> "hi3519-crg"
>> as the compatible string if it's a whole block.
>>
>> what about writing like this？
>>
>> crg: clock-reset-controller@1201 {
>> compatible = "hisilicon,hi3519-crg";
>> }
>>
> 
> Ok, that's better.
> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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_3M9
>> +
>> +/* mux clocks */
>> +#define HI3519_FMC_MUX 32
>> +#define HI3519_I2C_MUX 33
>> +#define HI3519_UART_MUX34
>> +#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
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

Hi Arnd,
   Thank you very much for all your comments.

On 2015/12/3 17:36, Arnd Bergmann wrote:
> On Thursday 03 December 2015 10:44:28 Jiancheng Xue wrote:
> 
>> +
>> +/dts-v1/;
>> +#include "hi3519.dtsi"
>> +
>> +/ {
>> +model = "HiSilicon HI3519 DEMO Board";
>> +compatible = "hisilicon,hi3519";
>> +
>> +chosen {
>> +bootargs = "mem=64M console=ttyAMA0,115200 early_printk \
>> +root=/dev/mtdblock2 rootfstype=jffs2 \
>> +mtdparts=hi_sfc:1M(boot),4M(kernel),11M(rootfs)";
>> +};
> 
> Most of the arguments should be dropped and replaced with the respective
> DT properties in this file:
> 
> mem:  /memory (you have that already, but the size seems wrong)
> console:  /chosen/stdout-path
> early_printk: just drop this, maybe use "earlycon")
> root: this one is fine
> rootfstype:   should not be needed
> mtdparts: use nodes below the MTD device
> 

This chosen node is just for debug. The real parameters will be set at boot 
stage.
I will drop it.

>> +
>> +#include "skeleton.dtsi"
>> +#include 
>> +/ {
>> +aliases {
>> +serial0 = &uart0;
>> +};
> 
> Move this into the .dts file.

OK. Thank you.

> 
>> +
>> +uart0: uart@1210 {
> 
> rename to serial@1210

OK.

> 
>> +dual_timer1: dual_timer@12001000 {
>> +compatible = "arm,sp804", "arm,primecell";
>> +interrupts = <0 66 4>, <0 67 4>;
>> +reg = <0x12001000 0x1000>;
>> +clocks = <&crg HI3519_FIXED_3M>;
>> +status = "disable";
>> +};
> 
> rename to timer@12001000

OK.

> 
>> +sysctrl: system-controller@1202 {
>> +compatible = "hisilicon,sysctrl";
>> +reg = <0x1202 0x1000>;
>> +reboot-offset = <0x4>;
>> +};
> 
> Is this one identical to the one in hip04?
> 
> If not, pick a new unique compatible string

Yes. It's compatible with the one in hip04.

> 
>> +
>> +crg: crg@1201 {
>> +compatible = "hisilicon,hi3519-crg";
> 
> 
> what is a "crg"? Is there a standard name for these?

The "crg" means Clock and Reset Generator. It's a block which supplies clock
and reset signals for other modules in the chip. I used "hi3519-clock"
in last version patch. Rob Herring suggested that it's better to use 
"hi3519-crg"
as the compatible string if it's a whole block.

what about writing like this？

crg: clock-reset-controller@1201 {
compatible = "hisilicon,hi3519-crg";
}

> 
>   Arnd
> 
> .
> 

Jiancheng
.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] clk: hi3519: add CRG driver for hisilicon hi3519 soc

[xuejiancheng]

Hello Rob,

Thanks for your suggestions!

On 2015/12/1 4:35, Rob Herring wrote:
> On Sat, Nov 28, 2015 at 03:13:26PM +0800, Jiancheng Xue wrote:
>> The CRG(Clock and Reset Generator) module provides
>> clock and reset signals for other modules in hi3519 soc.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-clock.txt |  46 +++
>>  drivers/clk/hisilicon/Makefile |   1 +
>>  drivers/clk/hisilicon/clk-hi3519.c | 130 ++
>>  drivers/clk/hisilicon/reset.c  | 149 
>> +
>>  drivers/clk/hisilicon/reset.h  |  25 
>>  include/dt-bindings/clock/hi3519-clock.h   |  78 +++
> 
> It is preferred to put binding doc and header in separate patch.

I will put them in separate patch in next version. Thank you!

> 
>>  6 files changed, 429 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-clock.txt
>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>  create mode 100644 include/dt-bindings/clock/hi3519-clock.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-clock.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-clock.txt
>> new file mode 100644
>> index 000..9fea878
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hi3519-clock.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-clock" - controller compatible with Hi3519 SoC.
> 
> Use -crg rather than -clock if that is the block name.

I agreed with you! It is proper to use -crg.

> 
>> +
>> +- 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 .
>> +
>> +- #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-clock";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@0x1211 {
>> +compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xE4 0>;
>> +};
> 
> 
>> diff --git a/drivers/clk/hisilicon/reset.h b/drivers/clk/hisilicon/reset.h
>> new file mode 100644
>> index 000..74bea4e
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * 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 .
>> + */
>> +
>> +#ifndef __HISI_RESET_H
>> +#define __HISI_RESET_H
>> +
>> +#include 
>> +
>> +int __init hisi_reset_init(struct device_node *np, int nr_rsts);
>> +
>> +#endif  /* __HISI_RESET_H */
>> diff --git a/include/dt-bindings/clock/hi3519-clock.h 
>> b/include/dt-bindings/clock/hi3519-clock.h
>> new file mode 100644
>> index 000..2e08666
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/hi3519-clock.h
>> @@ -0,0 +1,78 @@
>> +/*
>> + * 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 prog