[PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Bintian Wang]

Hi6220 is one mobile solution of Hisilicon, this patchset contains
initial support for Hi6220 SoC and HiKey development board, which
supports octal ARM Cortex A53 cores. Initial support is minimal and
includes just the arch configuration, clock driver, device tree
configuration.

PSCI is enabled in device tree and there is no problem to boot all the
octal cores, and the CPU hotplug is also working now, you can download
and compile the latest firmware based on the following link to run this
patch set:
https://github.com/96boards/documentation/wiki/UEFI 

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts 

Changes v3:
* Verified the CPU hotplug based on the new released firmware
* Redefined the compatible strings of four system controllers in dts 
* Setting COMMON_CLK_HI6220 to a bool symbol
* Keep CONFGI_ARCH_HISI sorted alphabetically

Changes v2:
* Split the DT bindings documents into earlier patches
* Change SMP enable method from spin-table to PSCI in device tree
* Remove "clock-frequency" from armv8-timer device node in device tree
* Add more description about Hisilicon designed system controllers
  in DT bindings document
* Enable high speed clock on UART1 mux
* Other changes based on the discussion in the mailing list:
  https://lkml.org/lkml/2015/2/5/147

Bintian Wang (5):
  arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig
  arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC
  clk: hi6220: Document devicetree bindings for hi6220 clock
  clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
  arm64: dts: Add dts files for Hisilicon Hi6220 SoC

 .../bindings/arm/hisilicon/hisilicon.txt           |  87 ++++++
 .../devicetree/bindings/clock/hi6220-clock.txt     |  34 +++
 arch/arm64/Kconfig                                 |   5 +
 arch/arm64/boot/dts/Makefile                       |   1 +
 arch/arm64/boot/dts/hisilicon/Makefile             |   5 +
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  31 +++
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 172 ++++++++++++
 arch/arm64/configs/defconfig                       |   1 +
 drivers/clk/Kconfig                                |   2 +
 drivers/clk/Makefile                               |   4 +-
 drivers/clk/hisilicon/Kconfig                      |   6 +
 drivers/clk/hisilicon/Makefile                     |   3 +-
 drivers/clk/hisilicon/clk-hi6220.c                 | 292 +++++++++++++++++++++
 drivers/clk/hisilicon/clk.c                        |  29 ++
 drivers/clk/hisilicon/clk.h                        |  17 ++
 drivers/clk/hisilicon/clkdivider-hi6220.c          | 273 +++++++++++++++++++
 include/dt-bindings/clock/hi6220-clock.h           | 173 ++++++++++++
 17 files changed, 1131 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt
 create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile
 create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
 create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi
 create mode 100644 drivers/clk/hisilicon/Kconfig
 create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
 create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c
 create mode 100644 include/dt-bindings/clock/hi6220-clock.h

-- 
1.9.1

[PATCH v4 2/5] arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC

[Bintian Wang]

This patch adds documentation for the devicetree bindings used by the
DT files of Hisilicon hi6220 SoC mobile platform.

Signed-off-by: Bintian Wang <bintian.wang@huawei.com>
---
 .../bindings/arm/hisilicon/hisilicon.txt           | 87 ++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
index 35b1bd4..f67d0f3 100644
--- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
+++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
@@ -1,5 +1,8 @@
 Hisilicon Platforms Device Tree Bindings
 ----------------------------------------------------
+Hi6220 SoC
+Required root node properties:
+	- compatible = "hisilicon,hi6220";
 
 Hi4511 Board
 Required root node properties:
@@ -13,6 +16,9 @@ HiP01 ca9x2 Board
 Required root node properties:
 	- compatible = "hisilicon,hip01-ca9x2";
 
+HiKey Board
+Required root node properties:
+	- compatible = "hisilicon,hi6220-hikey", "hisilicon,hi6220";
 
 Hisilicon system controller
 
@@ -41,6 +47,87 @@ Example:
 	};
 
 -----------------------------------------------------------------------
+Hisilicon Hi6220 system controller
+
+Required properties:
+- compatible : "hisilicon,hi6220-sysctrl"
+- reg : Register address and size
+- #clock-cells: should be set to 1, many clock registers are defined
+  under this controller and this property must be present.
+
+Hisilicon designs this controller as one of the system controllers,
+its main functions are the same as Hisilicon system controller, but
+the register offset of some core modules are different.
+
+Example:
+	/*for Hi6220*/
+	sys_ctrl: sys_ctrl {
+		compatible = "hisilicon,hi6220-sysctrl", "syscon";
+		reg = <0x0 0xf7030000 0x0 0x2000>;
+		#clock-cells = <1>;
+	};
+
+
+Hisilicon Hi6220 Power Always ON domain controller
+
+Required properties:
+- compatible : "hisilicon,hi6220-aoctrl"
+- reg : Register address and size
+- #clock-cells: should be set to 1, many clock registers are defined
+  under this controller and this property must be present.
+
+Hisilicon designs this system controller to control the power always
+on domain for mobile platform.
+
+Example:
+	/*for Hi6220*/
+	ao_ctrl: ao_ctrl {
+		compatible = "hisilicon,hi6220-aoctrl", "syscon";
+		reg = <0x0 0xf7800000 0x0 0x2000>;
+		#clock-cells = <1>;
+	};
+
+
+Hisilicon Hi6220 Media domain controller
+
+Required properties:
+- compatible : "hisilicon,hi6220-mediactrl"
+- reg : Register address and size
+- #clock-cells: should be set to 1, many clock registers are defined
+  under this controller and this property must be present.
+
+Hisilicon designs this system controller to control the multimedia
+domain(e.g. codec, G3D ...) for mobile platform.
+
+Example:
+	/*for Hi6220*/
+	media_ctrl: media_ctrl {
+		compatible = "hisilicon,hi6220-mediactrl", "syscon";
+		reg = <0x0 0xf4410000 0x0 0x1000>;
+		#clock-cells = <1>;
+	};
+
+
+Hisilicon Hi6220 Power Management domain controller
+
+Required properties:
+- compatible : "hisilicon,hi6220-pmctrl"
+- reg : Register address and size
+- #clock-cells: should be set to 1, some clock registers are define
+  under this controller and this property must be present.
+
+Hisilicon designs this system controller to control the power management
+domain for mobile platform.
+
+Example:
+	/*for Hi6220*/
+	pm_ctrl: pm_ctrl {
+		compatible = "hisilicon,hi6220-pmctrl", "syscon";
+		reg = <0x0 0xf7032000 0x0 0x1000>;
+		#clock-cells = <1>;
+	};
+
+-----------------------------------------------------------------------
 Hisilicon HiP01 system controller
 
 Required properties:
-- 
1.9.1

Re: [PATCH v4 2/5] arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC

[Stephen Boyd]

On 05/05, Bintian Wang wrote:
> This patch adds documentation for the devicetree bindings used by the
> DT files of Hisilicon hi6220 SoC mobile platform.
> 
> Signed-off-by: Bintian Wang <bintian.wang@huawei.com>

Acked-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/5] arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC

[Bintian]

Hello Stephen,

On 2015/5/15 8:27, Stephen Boyd wrote:
> On 05/05, Bintian Wang wrote:
>> This patch adds documentation for the devicetree bindings used by the
>> DT files of Hisilicon hi6220 SoC mobile platform.
>>
>> Signed-off-by: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>
> Acked-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>
Thanks for your ACK!

Bintian

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

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Tyler Baker]

On 5 May 2015 at 05:06, Bintian Wang <bintian.wang@huawei.com> wrote:
> Hi6220 is one mobile solution of Hisilicon, this patchset contains
> initial support for Hi6220 SoC and HiKey development board, which
> supports octal ARM Cortex A53 cores. Initial support is minimal and
> includes just the arch configuration, clock driver, device tree
> configuration.
>
> PSCI is enabled in device tree and there is no problem to boot all the
> octal cores, and the CPU hotplug is also working now, you can download
> and compile the latest firmware based on the following link to run this
> patch set:
> https://github.com/96boards/documentation/wiki/UEFI
>
> Changes v4:
> * Rebase to kernel 4.1-rc1
> * Delete "arm,cortex-a15-gic" from the gic node in dts

Tested by: Tyler Baker <tyler.baker@linaro.org>

Built and booted all v4 patches in this series ontop a v4.1-rc1 based
tree. Tested with the UEFI load mentioned above, booting to a minimal
ramdisk userspace[0]. Confirmed all 8 CPUs were activated.

>
> Changes v3:
> * Verified the CPU hotplug based on the new released firmware
> * Redefined the compatible strings of four system controllers in dts
> * Setting COMMON_CLK_HI6220 to a bool symbol
> * Keep CONFGI_ARCH_HISI sorted alphabetically
>
> Changes v2:
> * Split the DT bindings documents into earlier patches
> * Change SMP enable method from spin-table to PSCI in device tree
> * Remove "clock-frequency" from armv8-timer device node in device tree
> * Add more description about Hisilicon designed system controllers
>   in DT bindings document
> * Enable high speed clock on UART1 mux
> * Other changes based on the discussion in the mailing list:
>   https://lkml.org/lkml/2015/2/5/147
>
> Bintian Wang (5):
>   arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig
>   arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC
>   clk: hi6220: Document devicetree bindings for hi6220 clock
>   clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
>   arm64: dts: Add dts files for Hisilicon Hi6220 SoC
>
>  .../bindings/arm/hisilicon/hisilicon.txt           |  87 ++++++
>  .../devicetree/bindings/clock/hi6220-clock.txt     |  34 +++
>  arch/arm64/Kconfig                                 |   5 +
>  arch/arm64/boot/dts/Makefile                       |   1 +
>  arch/arm64/boot/dts/hisilicon/Makefile             |   5 +
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  31 +++
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 172 ++++++++++++
>  arch/arm64/configs/defconfig                       |   1 +
>  drivers/clk/Kconfig                                |   2 +
>  drivers/clk/Makefile                               |   4 +-
>  drivers/clk/hisilicon/Kconfig                      |   6 +
>  drivers/clk/hisilicon/Makefile                     |   3 +-
>  drivers/clk/hisilicon/clk-hi6220.c                 | 292 +++++++++++++++++++++
>  drivers/clk/hisilicon/clk.c                        |  29 ++
>  drivers/clk/hisilicon/clk.h                        |  17 ++
>  drivers/clk/hisilicon/clkdivider-hi6220.c          | 273 +++++++++++++++++++
>  include/dt-bindings/clock/hi6220-clock.h           | 173 ++++++++++++
>  17 files changed, 1131 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt
>  create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile
>  create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>  create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>  create mode 100644 drivers/clk/hisilicon/Kconfig
>  create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
>  create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c
>  create mode 100644 include/dt-bindings/clock/hi6220-clock.h
>
> --
> 1.9.1
>

Cheers,

Tyler

[0] http://kernelci.org/boot/hi6220-hikey/job/testing/kernel/v4.1-rc1-5-gf609561/defconfig/defconfig/lab/lab-tbaker/?_id=5549541559b51417e999c5cd

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Bintian]

Thank you Tyler for testing this patch set.

On 2015/5/6 7:46, Tyler Baker wrote:
> On 5 May 2015 at 05:06, Bintian Wang <bintian.wang@huawei.com> wrote:
>> Hi6220 is one mobile solution of Hisilicon, this patchset contains
>> initial support for Hi6220 SoC and HiKey development board, which
>> supports octal ARM Cortex A53 cores. Initial support is minimal and
>> includes just the arch configuration, clock driver, device tree
>> configuration.
>>
>> PSCI is enabled in device tree and there is no problem to boot all the
>> octal cores, and the CPU hotplug is also working now, you can download
>> and compile the latest firmware based on the following link to run this
>> patch set:
>> https://github.com/96boards/documentation/wiki/UEFI
>>
>> Changes v4:
>> * Rebase to kernel 4.1-rc1
>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>
> Tested by: Tyler Baker <tyler.baker@linaro.org>
>
> Built and booted all v4 patches in this series ontop a v4.1-rc1 based
> tree. Tested with the UEFI load mentioned above, booting to a minimal
> ramdisk userspace[0]. Confirmed all 8 CPUs were activated.
>
>>
>> Changes v3:
>> * Verified the CPU hotplug based on the new released firmware
>> * Redefined the compatible strings of four system controllers in dts
>> * Setting COMMON_CLK_HI6220 to a bool symbol
>> * Keep CONFGI_ARCH_HISI sorted alphabetically
>>
>> Changes v2:
>> * Split the DT bindings documents into earlier patches
>> * Change SMP enable method from spin-table to PSCI in device tree
>> * Remove "clock-frequency" from armv8-timer device node in device tree
>> * Add more description about Hisilicon designed system controllers
>>    in DT bindings document
>> * Enable high speed clock on UART1 mux
>> * Other changes based on the discussion in the mailing list:
>>    https://lkml.org/lkml/2015/2/5/147
>>
>> Bintian Wang (5):
>>    arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig
>>    arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC
>>    clk: hi6220: Document devicetree bindings for hi6220 clock
>>    clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
>>    arm64: dts: Add dts files for Hisilicon Hi6220 SoC
>>
>>   .../bindings/arm/hisilicon/hisilicon.txt           |  87 ++++++
>>   .../devicetree/bindings/clock/hi6220-clock.txt     |  34 +++
>>   arch/arm64/Kconfig                                 |   5 +
>>   arch/arm64/boot/dts/Makefile                       |   1 +
>>   arch/arm64/boot/dts/hisilicon/Makefile             |   5 +
>>   arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  31 +++
>>   arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 172 ++++++++++++
>>   arch/arm64/configs/defconfig                       |   1 +
>>   drivers/clk/Kconfig                                |   2 +
>>   drivers/clk/Makefile                               |   4 +-
>>   drivers/clk/hisilicon/Kconfig                      |   6 +
>>   drivers/clk/hisilicon/Makefile                     |   3 +-
>>   drivers/clk/hisilicon/clk-hi6220.c                 | 292 +++++++++++++++++++++
>>   drivers/clk/hisilicon/clk.c                        |  29 ++
>>   drivers/clk/hisilicon/clk.h                        |  17 ++
>>   drivers/clk/hisilicon/clkdivider-hi6220.c          | 273 +++++++++++++++++++
>>   include/dt-bindings/clock/hi6220-clock.h           | 173 ++++++++++++
>>   17 files changed, 1131 insertions(+), 4 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt
>>   create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile
>>   create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>>   create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>   create mode 100644 drivers/clk/hisilicon/Kconfig
>>   create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
>>   create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c
>>   create mode 100644 include/dt-bindings/clock/hi6220-clock.h
>>
>> --
>> 1.9.1
>>
>
> Cheers,
>
> Tyler
>
> [0] http://kernelci.org/boot/hi6220-hikey/job/testing/kernel/v4.1-rc1-5-gf609561/defconfig/defconfig/lab/lab-tbaker/?_id=5549541559b51417e999c5cd
>
> .
>

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Will Deacon]

Hi Bintian,

On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
> Hi6220 is one mobile solution of Hisilicon, this patchset contains
> initial support for Hi6220 SoC and HiKey development board, which
> supports octal ARM Cortex A53 cores. Initial support is minimal and
> includes just the arch configuration, clock driver, device tree
> configuration.
> 
> PSCI is enabled in device tree and there is no problem to boot all the
> octal cores, and the CPU hotplug is also working now, you can download
> and compile the latest firmware based on the following link to run this
> patch set:
> https://github.com/96boards/documentation/wiki/UEFI 
> 
> Changes v4:
> * Rebase to kernel 4.1-rc1
> * Delete "arm,cortex-a15-gic" from the gic node in dts 

I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
above.

The good news is that the thing booted and all the cores entered at EL2.
Thanks!

The bad news is that running hackbench quickly got the *heatsink*
temperature to 73 degress C and rising (measured with an infrared
thermometer).

So my question is, does this SoC have an automatic thermal cut out? Whilst
I'm all for merging enabling code into the kernel, if it really relies on
the kernel to stop it from catching fire, maybe it's not a great idea
putting these patches into people's hands just yet.

Will

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Bintian]

Hi Will,

On 2015/5/7 17:02, Will Deacon wrote:
> Hi Bintian,
>
> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>> Hi6220 is one mobile solution of Hisilicon, this patchset contains
>> initial support for Hi6220 SoC and HiKey development board, which
>> supports octal ARM Cortex A53 cores. Initial support is minimal and
>> includes just the arch configuration, clock driver, device tree
>> configuration.
>>
>> PSCI is enabled in device tree and there is no problem to boot all the
>> octal cores, and the CPU hotplug is also working now, you can download
>> and compile the latest firmware based on the following link to run this
>> patch set:
>> https://github.com/96boards/documentation/wiki/UEFI
>>
>> Changes v4:
>> * Rebase to kernel 4.1-rc1
>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>
> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
> above.
>
> The good news is that the thing booted and all the cores entered at EL2.
> Thanks!
Really thank you very much for testing this patch set.

>
> The bad news is that running hackbench quickly got the *heatsink*
> temperature to 73 degress C and rising (measured with an infrared
> thermometer).
This patch set is just for booting the small system, if you want to
test the temperature, I think you should using the HiKey released
version (https://www.96boards.org/).

This patch is just for the small system, and not include those drivers
for adjusting the CPU frequency, thermal control and so on. After this
patch is merged, all those drivers will be submitted later.

>
> So my question is, does this SoC have an automatic thermal cut out? Whilst
> I'm all for merging enabling code into the kernel, if it really relies on
> the kernel to stop it from catching fire, maybe it's not a great idea
> putting these patches into people's hands just yet.
Hikey is a low cost board, I think it doesn't have an automatic thermal
cut out; I always use HiKey to test my patch, in the normal case, 
temperature is not a problem.

Thanks,

Bintian
>
> Will
>
> .
>

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Will Deacon]

On Thu, May 07, 2015 at 10:29:03AM +0100, Bintian wrote:
> On 2015/5/7 17:02, Will Deacon wrote:
> > On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
> >> Hi6220 is one mobile solution of Hisilicon, this patchset contains
> >> initial support for Hi6220 SoC and HiKey development board, which
> >> supports octal ARM Cortex A53 cores. Initial support is minimal and
> >> includes just the arch configuration, clock driver, device tree
> >> configuration.
> >>
> >> PSCI is enabled in device tree and there is no problem to boot all the
> >> octal cores, and the CPU hotplug is also working now, you can download
> >> and compile the latest firmware based on the following link to run this
> >> patch set:
> >> https://github.com/96boards/documentation/wiki/UEFI
> >>
> >> Changes v4:
> >> * Rebase to kernel 4.1-rc1
> >> * Delete "arm,cortex-a15-gic" from the gic node in dts
> >
> > I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
> > above.
> >
> > The good news is that the thing booted and all the cores entered at EL2.
> > Thanks!
> Really thank you very much for testing this patch set.

Feel free to add my tested-by if you like.

> > The bad news is that running hackbench quickly got the *heatsink*
> > temperature to 73 degress C and rising (measured with an infrared
> > thermometer).
> This patch set is just for booting the small system, if you want to
> test the temperature, I think you should using the HiKey released
> version (https://www.96boards.org/).

I'm not really interested in the temperature numbers, but I am interested
in the board not melting and potentially setting fire to my desk.

> This patch is just for the small system, and not include those drivers
> for adjusting the CPU frequency, thermal control and so on. After this
> patch is merged, all those drivers will be submitted later.

Should those drivers *really* exist only in the kernel? What happens if
the kernel panics for some other reason? You'll basically have 8 spinning
cores and no sensible way to handle the thermal interrupt.

Shouldn't there be something in the secure firmware as a last resort?

> > So my question is, does this SoC have an automatic thermal cut out? Whilst
> > I'm all for merging enabling code into the kernel, if it really relies on
> > the kernel to stop it from catching fire, maybe it's not a great idea
> > putting these patches into people's hands just yet.
> Hikey is a low cost board, I think it doesn't have an automatic thermal
> cut out; I always use HiKey to test my patch, in the normal case, 
> temperature is not a problem.

I don't see why the cost has anything to do with this issue; any money I
save on the board will quickly be re-invested in my increased insurance
premium.

All I think we need is for secure software to keep an eye on the temperature
and hit the power controller if it goes over some `fatal' threshold.
Ideally, you'd be able to use a secure interrupt for this, but I suspect
that you don't have the right hardware features for that (please correct me
if I'm wrong). An alternative would be to hang something off a secure timer
and get the firmware to check the board temperature on some low-frequency
periodic tick.

Will

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Leo Yan]

On Thu, May 07, 2015 at 12:25:38PM +0100, Will Deacon wrote:
> On Thu, May 07, 2015 at 10:29:03AM +0100, Bintian wrote:
> > On 2015/5/7 17:02, Will Deacon wrote:
> > > On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:

[...]

> > > The bad news is that running hackbench quickly got the *heatsink*
> > > temperature to 73 degress C and rising (measured with an infrared
> > > thermometer).
> > This patch set is just for booting the small system, if you want to
> > test the temperature, I think you should using the HiKey released
> > version (https://www.96boards.org/).
> 
> I'm not really interested in the temperature numbers, but I am interested
> in the board not melting and potentially setting fire to my desk.

All cpus will be initialized w/t 800MHz, suppose the dynamic leakage is high;
w/t Bintian's patches, suppose the cpuidle also has not been enabled;
so there also have static leakage even cpus run into "wfi" state.

I also have not exactly power data for SoC, but suppose the leakage is
high.

> > This patch is just for the small system, and not include those drivers
> > for adjusting the CPU frequency, thermal control and so on. After this
> > patch is merged, all those drivers will be submitted later.
> 
> Should those drivers *really* exist only in the kernel? What happens if
> the kernel panics for some other reason? You'll basically have 8 spinning
> cores and no sensible way to handle the thermal interrupt.
> 
> Shouldn't there be something in the secure firmware as a last resort?

We are integrate Hisilicon's MCU firmware, which is the similiar
module w/t Juno's SCP. It will support mainly two functionality:

- CPU Frequency Scaling;

  CPUFreq driver has been merged yet. [2]

  For CPU frequency scaling part, Due MCU FW has some updating from
  hisilicon, so we need re-work cpu clock driver. [1]

- CPUIdle and system's low power state;

  This part need integrate ARM-TF w/t MCU FW to support PSCI;

> > > So my question is, does this SoC have an automatic thermal cut out? Whilst
> > > I'm all for merging enabling code into the kernel, if it really relies on
> > > the kernel to stop it from catching fire, maybe it's not a great idea
> > > putting these patches into people's hands just yet.
> > Hikey is a low cost board, I think it doesn't have an automatic thermal
> > cut out; I always use HiKey to test my patch, in the normal case, 
> > temperature is not a problem.
> 
> I don't see why the cost has anything to do with this issue; any money I
> save on the board will quickly be re-invested in my increased insurance
> premium.
> 
> All I think we need is for secure software to keep an eye on the temperature
> and hit the power controller if it goes over some `fatal' threshold.
> Ideally, you'd be able to use a secure interrupt for this, but I suspect
> that you don't have the right hardware features for that (please correct me
> if I'm wrong). An alternative would be to hang something off a secure timer
> and get the firmware to check the board temperature on some low-frequency
> periodic tick.

Now we are using thermal framework to monitor thermal and if
temperature hit the trip point it will take cpu as cooling device and
set limitation for cpu's max frequency. [3]

[1] http://archive.arm.linux.org.uk/lurker/message/20150326.111335.0d484b89.en.html
[2] http://archive.arm.linux.org.uk/lurker/message/20150330.052637.16e0f3f4.en.html
[3] http://archive.arm.linux.org.uk/lurker/message/20150424.035121.80e0e24b.en.html

Thanks,
Leo Yan

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Bintian]

Hi Will,

On 2015/5/7 19:25, Will Deacon wrote:
> On Thu, May 07, 2015 at 10:29:03AM +0100, Bintian wrote:
>> On 2015/5/7 17:02, Will Deacon wrote:
>>> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>>>> Hi6220 is one mobile solution of Hisilicon, this patchset contains
>>>> initial support for Hi6220 SoC and HiKey development board, which
>>>> supports octal ARM Cortex A53 cores. Initial support is minimal and
>>>> includes just the arch configuration, clock driver, device tree
>>>> configuration.
>>>>
>>>> PSCI is enabled in device tree and there is no problem to boot all the
>>>> octal cores, and the CPU hotplug is also working now, you can download
>>>> and compile the latest firmware based on the following link to run this
>>>> patch set:
>>>> https://github.com/96boards/documentation/wiki/UEFI
>>>>
>>>> Changes v4:
>>>> * Rebase to kernel 4.1-rc1
>>>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>>>
>>> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
>>> above.
>>>
>>> The good news is that the thing booted and all the cores entered at EL2.
>>> Thanks!
>> Really thank you very much for testing this patch set.
>
> Feel free to add my tested-by if you like.
Sure, I will add and prepare the next version soon.

>
>>> The bad news is that running hackbench quickly got the *heatsink*
>>> temperature to 73 degress C and rising (measured with an infrared
>>> thermometer).
>> This patch set is just for booting the small system, if you want to
>> test the temperature, I think you should using the HiKey released
>> version (https://www.96boards.org/).
>
> I'm not really interested in the temperature numbers, but I am interested
> in the board not melting and potentially setting fire to my desk.
>
>> This patch is just for the small system, and not include those drivers
>> for adjusting the CPU frequency, thermal control and so on. After this
>> patch is merged, all those drivers will be submitted later.
>
> Should those drivers *really* exist only in the kernel? What happens if
> the kernel panics for some other reason? You'll basically have 8 spinning
> cores and no sensible way to handle the thermal interrupt.
>
> Shouldn't there be something in the secure firmware as a last resort?
>
>>> So my question is, does this SoC have an automatic thermal cut out? Whilst
>>> I'm all for merging enabling code into the kernel, if it really relies on
>>> the kernel to stop it from catching fire, maybe it's not a great idea
>>> putting these patches into people's hands just yet.
>> Hikey is a low cost board, I think it doesn't have an automatic thermal
>> cut out; I always use HiKey to test my patch, in the normal case,
>> temperature is not a problem.
>
> I don't see why the cost has anything to do with this issue; any money I
> save on the board will quickly be re-invested in my increased insurance
> premium.
>
> All I think we need is for secure software to keep an eye on the temperature
> and hit the power controller if it goes over some `fatal' threshold.
> Ideally, you'd be able to use a secure interrupt for this, but I suspect
> that you don't have the right hardware features for that (please correct me
> if I'm wrong). An alternative would be to hang something off a secure timer
> and get the firmware to check the board temperature on some low-frequency
> periodic tick.
If there is exception occurred on A core, there are two methods to
handle it:
(1) Delay for a period of time, watchdog will trigger the system reset.
(2) If the temperature is over 105 degree, the CPU will trigger reset(I
guess it's chip level).

Thanks,

Bintian

>
> Will
>
> .
>

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Will Deacon]

On Thu, May 07, 2015 at 01:01:47PM +0100, Bintian wrote:
> On 2015/5/7 19:25, Will Deacon wrote:
> > On Thu, May 07, 2015 at 10:29:03AM +0100, Bintian wrote:
> >> Hikey is a low cost board, I think it doesn't have an automatic thermal
> >> cut out; I always use HiKey to test my patch, in the normal case,
> >> temperature is not a problem.
> >
> > I don't see why the cost has anything to do with this issue; any money I
> > save on the board will quickly be re-invested in my increased insurance
> > premium.
> >
> > All I think we need is for secure software to keep an eye on the temperature
> > and hit the power controller if it goes over some `fatal' threshold.
> > Ideally, you'd be able to use a secure interrupt for this, but I suspect
> > that you don't have the right hardware features for that (please correct me
> > if I'm wrong). An alternative would be to hang something off a secure timer
> > and get the firmware to check the board temperature on some low-frequency
> > periodic tick.
> If there is exception occurred on A core, there are two methods to
> handle it:
> (1) Delay for a period of time, watchdog will trigger the system reset.
> (2) If the temperature is over 105 degree, the CPU will trigger reset(I
> guess it's chip level).

Aha, so now you're saying that there *is* a hardware shut-off at 105
degrees, regardless of what the kernel is doing? If that's the case, then
we're fine and the current patches make sense in isolation.

Cheers,

Will

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Bintian]

Hello Will,

On 2015/5/7 20:57, Will Deacon wrote:
> On Thu, May 07, 2015 at 01:01:47PM +0100, Bintian wrote:
>> On 2015/5/7 19:25, Will Deacon wrote:
>>> On Thu, May 07, 2015 at 10:29:03AM +0100, Bintian wrote:
>>>> Hikey is a low cost board, I think it doesn't have an automatic thermal
>>>> cut out; I always use HiKey to test my patch, in the normal case,
>>>> temperature is not a problem.
>>>
>>> I don't see why the cost has anything to do with this issue; any money I
>>> save on the board will quickly be re-invested in my increased insurance
>>> premium.
>>>
>>> All I think we need is for secure software to keep an eye on the temperature
>>> and hit the power controller if it goes over some `fatal' threshold.
>>> Ideally, you'd be able to use a secure interrupt for this, but I suspect
>>> that you don't have the right hardware features for that (please correct me
>>> if I'm wrong). An alternative would be to hang something off a secure timer
>>> and get the firmware to check the board temperature on some low-frequency
>>> periodic tick.
>> If there is exception occurred on A core, there are two methods to
>> handle it:
>> (1) Delay for a period of time, watchdog will trigger the system reset.
>> (2) If the temperature is over 105 degree, the CPU will trigger reset(I
>> guess it's chip level).
>
> Aha, so now you're saying that there *is* a hardware shut-off at 105
> degrees, regardless of what the kernel is doing?
Yes!

> If that's the case, then
> we're fine and the current patches make sense in isolation.
Thank you very much and I need your ack in next version :)
I am preparing the next version and will send out soon.

Thanks,

Bintian

>
> Cheers,
>
> Will
>
> .
>

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Haojian Zhuang]

On 7 May 2015 at 17:02, Will Deacon <will.deacon@arm.com> wrote:
> Hi Bintian,
>
> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>> Hi6220 is one mobile solution of Hisilicon, this patchset contains
>> initial support for Hi6220 SoC and HiKey development board, which
>> supports octal ARM Cortex A53 cores. Initial support is minimal and
>> includes just the arch configuration, clock driver, device tree
>> configuration.
>>
>> PSCI is enabled in device tree and there is no problem to boot all the
>> octal cores, and the CPU hotplug is also working now, you can download
>> and compile the latest firmware based on the following link to run this
>> patch set:
>> https://github.com/96boards/documentation/wiki/UEFI
>>
>> Changes v4:
>> * Rebase to kernel 4.1-rc1
>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>
> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
> above.
>
> The good news is that the thing booted and all the cores entered at EL2.
> Thanks!
>
> The bad news is that running hackbench quickly got the *heatsink*
> temperature to 73 degress C and rising (measured with an infrared
> thermometer).

Because you're just testing with minimum patch set. If you can choose
our release
on v3.10 & v3.18, you can observe lower temperature. Because we have thermal
framework and cpufreq driver. We're also upstreaming the thermal driver, but
it's not merged yet.

>
> So my question is, does this SoC have an automatic thermal cut out?

It's a low cost board. This feature is implemente by software. So we
need thermal
driver and cpufreq driver.

Regards
Haojian

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Jorge Ramirez-Ortiz]

On 05/07/2015 05:33 AM, Haojian Zhuang wrote:
> On 7 May 2015 at 17:02, Will Deacon <will.deacon@arm.com> wrote:
>> Hi Bintian,
>>
>> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>>> Hi6220 is one mobile solution of Hisilicon, this patchset contains
>>> initial support for Hi6220 SoC and HiKey development board, which
>>> supports octal ARM Cortex A53 cores. Initial support is minimal and
>>> includes just the arch configuration, clock driver, device tree
>>> configuration.
>>>
>>> PSCI is enabled in device tree and there is no problem to boot all the
>>> octal cores, and the CPU hotplug is also working now, you can download
>>> and compile the latest firmware based on the following link to run this
>>> patch set:
>>> https://github.com/96boards/documentation/wiki/UEFI
>>>
>>> Changes v4:
>>> * Rebase to kernel 4.1-rc1
>>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
>> above.
>>
>> The good news is that the thing booted and all the cores entered at EL2.
>> Thanks!
>>
>> The bad news is that running hackbench quickly got the *heatsink*
>> temperature to 73 degress C and rising (measured with an infrared
>> thermometer).
> Because you're just testing with minimum patch set. If you can choose
> our release
> on v3.10 & v3.18, you can observe lower temperature. Because we have thermal
> framework and cpufreq driver. We're also upstreaming the thermal driver, but
> it's not merged yet.

the thermal driver is now on v4 (see below)
https://lkml.org/lkml/2015/4/24/1

>
>> So my question is, does this SoC have an automatic thermal cut out?
> It's a low cost board. This feature is implemente by software. So we
> need thermal
> driver and cpufreq driver.
>
> Regards
> Haojian

[PATCH v4 1/5] arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig

[Bintian Wang]

This patch introduces ARCH_HISI to enable Hisilicon SoC family in
Kconfig and defconfig.

Signed-off-by: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Haojian Zhuang <haojian.zhuang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Wei Xu <xuwei5-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
---
 arch/arm64/Kconfig           | 5 +++++
 arch/arm64/configs/defconfig | 1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4269dba..2af5efe 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -180,6 +180,11 @@ config ARCH_FSL_LS2085A
 	help
 	  This enables support for Freescale LS2085A SOC.
 
+config ARCH_HISI
+	bool "Hisilicon SoC Family"
+	help
+	  This enables support for Hisilicon ARMv8 SoC family
+
 config ARCH_MEDIATEK
 	bool "Mediatek MT65xx & MT81xx ARMv8 SoC"
 	select ARM_GIC
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 2ed7449..1d293ea 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -33,6 +33,7 @@ CONFIG_MODULE_UNLOAD=y
 # CONFIG_IOSCHED_DEADLINE is not set
 CONFIG_ARCH_EXYNOS7=y
 CONFIG_ARCH_FSL_LS2085A=y
+CONFIG_ARCH_HISI=y
 CONFIG_ARCH_MEDIATEK=y
 CONFIG_ARCH_SEATTLE=y
 CONFIG_ARCH_TEGRA=y
-- 
1.9.1

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

[PATCH v4 3/5] clk: hi6220: Document devicetree bindings for hi6220 clock

[Bintian Wang]

Document DT files bindings for Hisilicon hi6220 clock.

Signed-off-by: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Haojian Zhuang <haojian.zhuang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/clock/hi6220-clock.txt     | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/hi6220-clock.txt b/Documentation/devicetree/bindings/clock/hi6220-clock.txt
new file mode 100644
index 0000000..53ddb19
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/hi6220-clock.txt
@@ -0,0 +1,34 @@
+* Hisilicon Hi6220 Clock Controller
+
+Clock control registers reside in different Hi6220 system controllers,
+please refer the following document to know more about the binding rules
+for these system controllers:
+
+Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
+
+Required Properties:
+
+- compatible: the compatible should be one of the following strings to
+	indicate the clock controller functionality.
+
+	- "hisilicon,hi6220-aoctrl"
+	- "hisilicon,hi6220-sysctrl"
+	- "hisilicon,hi6220-mediactrl"
+	- "hisilicon,hi6220-pmctrl"
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- #clock-cells: should be 1.
+
+For example:
+	sys_ctrl: sys_ctrl {
+		compatible = "hisilicon,hi6220-sysctrl", "syscon";
+		reg = <0x0 0xf7030000 0x0 0x2000>;
+		#clock-cells = <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/hi6220-clock.h>.
-- 
1.9.1

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

Re: [PATCH v4 3/5] clk: hi6220: Document devicetree bindings for hi6220 clock

[Stephen Boyd]

On 05/05, Bintian Wang wrote:
> Document DT files bindings for Hisilicon hi6220 clock.
> 
> Signed-off-by: Bintian Wang <bintian.wang@huawei.com>
> Reviewed-by: Haojian Zhuang <haojian.zhuang@linaro.org>

Acked-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC

[Bintian Wang]

Add clock drivers for hi6220 SoC, this driver controls the SoC
registers to supply different clocks to different IPs in the SoC.

We add one divider clock for hi6220 because the divider in hi6220
also has a mask bit but it doesnot obey the rule defined by flag
"CLK_DIVIDER_HIWORD_MASK", we can not get index of the mask bit by
left shift fixed bits (e.g. 16 bits), so we add this divider clock
to handle it.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Haojian Zhuang <haojian.zhuang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/clk/Kconfig                       |   2 +
 drivers/clk/Makefile                      |   4 +-
 drivers/clk/hisilicon/Kconfig             |   6 +
 drivers/clk/hisilicon/Makefile            |   3 +-
 drivers/clk/hisilicon/clk-hi6220.c        | 292 ++++++++++++++++++++++++++++++
 drivers/clk/hisilicon/clk.c               |  29 +++
 drivers/clk/hisilicon/clk.h               |  17 ++
 drivers/clk/hisilicon/clkdivider-hi6220.c | 273 ++++++++++++++++++++++++++++
 include/dt-bindings/clock/hi6220-clock.h  | 173 ++++++++++++++++++
 9 files changed, 795 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/hisilicon/Kconfig
 create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
 create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c
 create mode 100644 include/dt-bindings/clock/hi6220-clock.h

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9897f35..935c44b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -152,6 +152,8 @@ config COMMON_CLK_CDCE706
 
 source "drivers/clk/qcom/Kconfig"
 
+source "drivers/clk/hisilicon/Kconfig"
+
 endmenu
 
 source "drivers/clk/bcm/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25..9719954 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -47,9 +47,7 @@ obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
 obj-$(CONFIG_COMMON_CLK_AT91)		+= at91/
 obj-$(CONFIG_ARCH_BCM_MOBILE)		+= bcm/
 obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
-obj-$(CONFIG_ARCH_HI3xxx)		+= hisilicon/
-obj-$(CONFIG_ARCH_HIP04)		+= hisilicon/
-obj-$(CONFIG_ARCH_HIX5HD2)		+= hisilicon/
+obj-$(CONFIG_ARCH_HISI)			+= hisilicon/
 obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
 ifeq ($(CONFIG_COMMON_CLK), y)
 obj-$(CONFIG_ARCH_MMP)			+= mmp/
diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
new file mode 100644
index 0000000..8034739
--- /dev/null
+++ b/drivers/clk/hisilicon/Kconfig
@@ -0,0 +1,6 @@
+config COMMON_CLK_HI6220
+	bool "Hi6220 Clock Driver"
+	depends on OF && ARCH_HISI
+	default y
+	help
+	  Build the Hisilicon Hi6220 clock driver based on the common clock framework.
diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
index 038c02f..48f0116 100644
--- a/drivers/clk/hisilicon/Makefile
+++ b/drivers/clk/hisilicon/Makefile
@@ -2,8 +2,9 @@
 # Hisilicon Clock specific Makefile
 #
 
-obj-y	+= clk.o clkgate-separated.o
+obj-y	+= clk.o clkgate-separated.o clkdivider-hi6220.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
diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c
new file mode 100644
index 0000000..91b1cd7
--- /dev/null
+++ b/drivers/clk/hisilicon/clk-hi6220.c
@@ -0,0 +1,292 @@
+/*
+ * Hisilicon Hi6220 clock driver
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * Author: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+
+#include <dt-bindings/clock/hi6220-clock.h>
+
+#include "clk.h"
+
+
+/* clocks in AO (always on) controller */
+static struct hisi_fixed_rate_clock hi6220_fixed_rate_clks[] __initdata = {
+	{ HI6220_REF32K,	"ref32k",	NULL, CLK_IS_ROOT, 32764,     },
+	{ HI6220_CLK_TCXO,	"clk_tcxo",	NULL, CLK_IS_ROOT, 19200000,  },
+	{ HI6220_MMC1_PAD,	"mmc1_pad",	NULL, CLK_IS_ROOT, 100000000, },
+	{ HI6220_MMC2_PAD,	"mmc2_pad",	NULL, CLK_IS_ROOT, 100000000, },
+	{ HI6220_MMC0_PAD,	"mmc0_pad",	NULL, CLK_IS_ROOT, 200000000, },
+	{ HI6220_PLL_BBP,	"bbppll0",	NULL, CLK_IS_ROOT, 245760000, },
+	{ HI6220_PLL_GPU,	"gpupll",	NULL, CLK_IS_ROOT, 1000000000,},
+	{ HI6220_PLL1_DDR,	"ddrpll1",	NULL, CLK_IS_ROOT, 1066000000,},
+	{ HI6220_PLL_SYS,	"syspll",	NULL, CLK_IS_ROOT, 1200000000,},
+	{ HI6220_PLL_SYS_MEDIA,	"media_syspll",	NULL, CLK_IS_ROOT, 1200000000,},
+	{ HI6220_DDR_SRC,	"ddr_sel_src",  NULL, CLK_IS_ROOT, 1200000000,},
+	{ HI6220_PLL_MEDIA,	"media_pll",    NULL, CLK_IS_ROOT, 1440000000,},
+	{ HI6220_PLL_DDR,	"ddrpll0",      NULL, CLK_IS_ROOT, 1600000000,},
+};
+
+static struct hisi_fixed_factor_clock hi6220_fixed_factor_clks[] __initdata = {
+	{ HI6220_300M,         "clk_300m",    "syspll",          1, 4, 0, },
+	{ HI6220_150M,         "clk_150m",    "clk_300m",        1, 2, 0, },
+	{ HI6220_PICOPHY_SRC,  "picophy_src", "clk_150m",        1, 4, 0, },
+	{ HI6220_MMC0_SRC_SEL, "mmc0srcsel",  "mmc0_sel",        1, 8, 0, },
+	{ HI6220_MMC1_SRC_SEL, "mmc1srcsel",  "mmc1_sel",        1, 8, 0, },
+	{ HI6220_MMC2_SRC_SEL, "mmc2srcsel",  "mmc2_sel",        1, 8, 0, },
+	{ HI6220_VPU_CODEC,    "vpucodec",    "codec_jpeg_aclk", 1, 2, 0, },
+	{ HI6220_MMC0_SMP,     "mmc0_sample", "mmc0_sel",        1, 8, 0, },
+	{ HI6220_MMC1_SMP,     "mmc1_sample", "mmc1_sel",        1, 8, 0, },
+	{ HI6220_MMC2_SMP,     "mmc2_sample", "mmc2_sel",        1, 8, 0, },
+};
+
+static struct hisi_gate_clock hi6220_separated_gate_clks_ao[] __initdata = {
+	{ HI6220_WDT0_PCLK,   "wdt0_pclk",   "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 12, 0, },
+	{ HI6220_WDT1_PCLK,   "wdt1_pclk",   "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 13, 0, },
+	{ HI6220_WDT2_PCLK,   "wdt2_pclk",   "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 14, 0, },
+	{ HI6220_TIMER0_PCLK, "timer0_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 15, 0, },
+	{ HI6220_TIMER1_PCLK, "timer1_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 16, 0, },
+	{ HI6220_TIMER2_PCLK, "timer2_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 17, 0, },
+	{ HI6220_TIMER3_PCLK, "timer3_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 18, 0, },
+	{ HI6220_TIMER4_PCLK, "timer4_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 19, 0, },
+	{ HI6220_TIMER5_PCLK, "timer5_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 20, 0, },
+	{ HI6220_TIMER6_PCLK, "timer6_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 21, 0, },
+	{ HI6220_TIMER7_PCLK, "timer7_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 22, 0, },
+	{ HI6220_TIMER8_PCLK, "timer8_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 23, 0, },
+	{ HI6220_UART0_PCLK,  "uart0_pclk",  "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 24, 0, },
+};
+
+static struct hisi_clock_data *clk_data_ao;
+
+static void __init hi6220_clk_ao_init(struct device_node *np)
+{
+	clk_data_ao = hisi_clk_init(np, HI6220_AO_NR_CLKS);
+	if (!clk_data_ao)
+		return;
+
+	hisi_clk_register_fixed_rate(hi6220_fixed_rate_clks,
+				ARRAY_SIZE(hi6220_fixed_rate_clks), clk_data_ao);
+
+	hisi_clk_register_fixed_factor(hi6220_fixed_factor_clks,
+				ARRAY_SIZE(hi6220_fixed_factor_clks), clk_data_ao);
+
+	hisi_clk_register_gate_sep(hi6220_separated_gate_clks_ao,
+				ARRAY_SIZE(hi6220_separated_gate_clks_ao), clk_data_ao);
+}
+CLK_OF_DECLARE(hi6220_clk_ao, "hisilicon,hi6220-aoctrl", hi6220_clk_ao_init);
+
+
+/* clocks in sysctrl */
+static const char *mmc0_mux0_p[] __initconst = { "pll_ddr_gate", "syspll", };
+static const char *mmc0_mux1_p[] __initconst = { "mmc0_mux0", "pll_media_gate", };
+static const char *mmc0_src_p[] __initconst = { "mmc0srcsel", "mmc0_div", };
+static const char *mmc1_mux0_p[] __initconst = { "pll_ddr_gate", "syspll", };
+static const char *mmc1_mux1_p[] __initconst = { "mmc1_mux0", "pll_media_gate", };
+static const char *mmc1_src_p[]  __initconst = { "mmc1srcsel", "mmc1_div", };
+static const char *mmc2_mux0_p[] __initconst = { "pll_ddr_gate", "syspll", };
+static const char *mmc2_mux1_p[] __initconst = { "mmc2_mux0", "pll_media_gate", };
+static const char *mmc2_src_p[]  __initconst = { "mmc2srcsel", "mmc2_div", };
+static const char *mmc0_sample_in[] __initconst = { "mmc0_sample", "mmc0_pad", };
+static const char *mmc1_sample_in[] __initconst = { "mmc1_sample", "mmc1_pad", };
+static const char *mmc2_sample_in[] __initconst = { "mmc2_sample", "mmc2_pad", };
+static const char *uart1_src[] __initconst = { "clk_tcxo", "clk_150m", };
+static const char *uart2_src[] __initconst = { "clk_tcxo", "clk_150m", };
+static const char *uart3_src[] __initconst = { "clk_tcxo", "clk_150m", };
+static const char *uart4_src[] __initconst = { "clk_tcxo", "clk_150m", };
+static const char *hifi_src[] __initconst = { "syspll", "pll_media_gate", };
+
+static struct hisi_gate_clock hi6220_separated_gate_clks_sys[] __initdata = {
+	{ HI6220_MMC0_CLK,      "mmc0_clk",      "mmc0_src",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 0,  0, },
+	{ HI6220_MMC0_CIUCLK,   "mmc0_ciuclk",   "mmc0_smp_in",    CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 0,  0, },
+	{ HI6220_MMC1_CLK,      "mmc1_clk",      "mmc1_src",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 1,  0, },
+	{ HI6220_MMC1_CIUCLK,   "mmc1_ciuclk",   "mmc1_smp_in",    CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 1,  0, },
+	{ HI6220_MMC2_CLK,      "mmc2_clk",      "mmc2_src",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 2,  0, },
+	{ HI6220_MMC2_CIUCLK,   "mmc2_ciuclk",   "mmc2_smp_in",    CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 2,  0, },
+	{ HI6220_USBOTG_HCLK,   "usbotg_hclk",   "clk_bus",        CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 4,  0, },
+	{ HI6220_CLK_PICOPHY,   "clk_picophy",   "cs_dapb",        CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x200, 5,  0, },
+	{ HI6220_HIFI,          "hifi_clk",      "hifi_div",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x210, 0,  0, },
+	{ HI6220_DACODEC_PCLK,  "dacodec_pclk",  "clk_bus",        CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x210, 5,  0, },
+	{ HI6220_EDMAC_ACLK,    "edmac_aclk",    "clk_bus",        CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x220, 2,  0, },
+	{ HI6220_CS_ATB,        "cs_atb",        "cs_atb_div",     CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 0,  0, },
+	{ HI6220_I2C0_CLK,      "i2c0_clk",      "clk_150m",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 1,  0, },
+	{ HI6220_I2C1_CLK,      "i2c1_clk",      "clk_150m",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 2,  0, },
+	{ HI6220_I2C2_CLK,      "i2c2_clk",      "clk_150m",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 3,  0, },
+	{ HI6220_I2C3_CLK,      "i2c3_clk",      "clk_150m",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 4,  0, },
+	{ HI6220_UART1_PCLK,    "uart1_pclk",    "uart1_src",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 5,  0, },
+	{ HI6220_UART2_PCLK,    "uart2_pclk",    "uart2_src",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 6,  0, },
+	{ HI6220_UART3_PCLK,    "uart3_pclk",    "uart3_src",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 7,  0, },
+	{ HI6220_UART4_PCLK,    "uart4_pclk",    "uart4_src",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 8,  0, },
+	{ HI6220_SPI_CLK,       "spi_clk",       "clk_150m",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 9,  0, },
+	{ HI6220_TSENSOR_CLK,   "tsensor_clk",   "clk_bus",        CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 12, 0, },
+	{ HI6220_MMU_CLK,       "mmu_clk",       "ddrc_axi1",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x240, 11, 0, },
+	{ HI6220_HIFI_SEL,      "hifi_sel",      "hifi_src",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 0,  0, },
+	{ HI6220_MMC0_SYSPLL,   "mmc0_syspll",   "syspll",         CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 1,  0, },
+	{ HI6220_MMC1_SYSPLL,   "mmc1_syspll",   "syspll",         CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 2,  0, },
+	{ HI6220_MMC2_SYSPLL,   "mmc2_syspll",   "syspll",         CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 3,  0, },
+	{ HI6220_MMC0_SEL,      "mmc0_sel",      "mmc0_mux1",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 6,  0, },
+	{ HI6220_MMC1_SEL,      "mmc1_sel",      "mmc1_mux1",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 7,  0, },
+	{ HI6220_BBPPLL_SEL,    "bbppll_sel",    "pll0_bbp_gate",  CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 9,  0, },
+	{ HI6220_MEDIA_PLL_SRC, "media_pll_src", "pll_media_gate", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 10, 0, },
+	{ HI6220_MMC2_SEL,      "mmc2_sel",      "mmc2_mux1",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 11, 0, },
+	{ HI6220_CS_ATB_SYSPLL, "cs_atb_syspll", "syspll",         CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 12, 0, },
+};
+
+static struct hisi_mux_clock hi6220_mux_clks_sys[] __initdata = {
+	{ HI6220_MMC0_SRC,    "mmc0_src",    mmc0_src_p,     ARRAY_SIZE(mmc0_src_p),     CLK_SET_RATE_PARENT, 0x4,   0,  1, 0, },
+	{ HI6220_MMC0_SMP_IN, "mmc0_smp_in", mmc0_sample_in, ARRAY_SIZE(mmc0_sample_in), CLK_SET_RATE_PARENT, 0x4,   0,  1, 0, },
+	{ HI6220_MMC1_SRC,    "mmc1_src",    mmc1_src_p,     ARRAY_SIZE(mmc1_src_p),     CLK_SET_RATE_PARENT, 0x4,   2,  1, 0, },
+	{ HI6220_MMC1_SMP_IN, "mmc1_smp_in", mmc1_sample_in, ARRAY_SIZE(mmc1_sample_in), CLK_SET_RATE_PARENT, 0x4,   2,  1, 0, },
+	{ HI6220_MMC2_SRC,    "mmc2_src",    mmc2_src_p,     ARRAY_SIZE(mmc2_src_p),     CLK_SET_RATE_PARENT, 0x4,   4,  1, 0, },
+	{ HI6220_MMC2_SMP_IN, "mmc2_smp_in", mmc2_sample_in, ARRAY_SIZE(mmc2_sample_in), CLK_SET_RATE_PARENT, 0x4,   4,  1, 0, },
+	{ HI6220_HIFI_SRC,    "hifi_src",    hifi_src,       ARRAY_SIZE(hifi_src),       CLK_SET_RATE_PARENT, 0x400, 0,  1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_UART1_SRC,   "uart1_src",   uart1_src,      ARRAY_SIZE(uart1_src),      CLK_SET_RATE_PARENT, 0x400, 1,  1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_UART2_SRC,   "uart2_src",   uart2_src,      ARRAY_SIZE(uart2_src),      CLK_SET_RATE_PARENT, 0x400, 2,  1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_UART3_SRC,   "uart3_src",   uart3_src,      ARRAY_SIZE(uart3_src),      CLK_SET_RATE_PARENT, 0x400, 3,  1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_UART4_SRC,   "uart4_src",   uart4_src,      ARRAY_SIZE(uart4_src),      CLK_SET_RATE_PARENT, 0x400, 4,  1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_MMC0_MUX0,   "mmc0_mux0",   mmc0_mux0_p,    ARRAY_SIZE(mmc0_mux0_p),    CLK_SET_RATE_PARENT, 0x400, 5,  1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_MMC1_MUX0,   "mmc1_mux0",   mmc1_mux0_p,    ARRAY_SIZE(mmc1_mux0_p),    CLK_SET_RATE_PARENT, 0x400, 11, 1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_MMC2_MUX0,   "mmc2_mux0",   mmc2_mux0_p,    ARRAY_SIZE(mmc2_mux0_p),    CLK_SET_RATE_PARENT, 0x400, 12, 1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_MMC0_MUX1,   "mmc0_mux1",   mmc0_mux1_p,    ARRAY_SIZE(mmc0_mux1_p),    CLK_SET_RATE_PARENT, 0x400, 13, 1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_MMC1_MUX1,   "mmc1_mux1",   mmc1_mux1_p,    ARRAY_SIZE(mmc1_mux1_p),    CLK_SET_RATE_PARENT, 0x400, 14, 1, CLK_MUX_HIWORD_MASK,},
+	{ HI6220_MMC2_MUX1,   "mmc2_mux1",   mmc2_mux1_p,    ARRAY_SIZE(mmc2_mux1_p),    CLK_SET_RATE_PARENT, 0x400, 15, 1, CLK_MUX_HIWORD_MASK,},
+};
+
+static struct hi6220_divider_clock hi6220_div_clks_sys[] __initdata = {
+	{ HI6220_CLK_BUS,     "clk_bus",     "clk_300m",      CLK_SET_RATE_PARENT, 0x490, 0,  4, 7, },
+	{ HI6220_MMC0_DIV,    "mmc0_div",    "mmc0_syspll",   CLK_SET_RATE_PARENT, 0x494, 0,  6, 7, },
+	{ HI6220_MMC1_DIV,    "mmc1_div",    "mmc1_syspll",   CLK_SET_RATE_PARENT, 0x498, 0,  6, 7, },
+	{ HI6220_MMC2_DIV,    "mmc2_div",    "mmc2_syspll",   CLK_SET_RATE_PARENT, 0x49c, 0,  6, 7, },
+	{ HI6220_HIFI_DIV,    "hifi_div",    "hifi_sel",      CLK_SET_RATE_PARENT, 0x4a0, 0,  4, 7, },
+	{ HI6220_BBPPLL0_DIV, "bbppll0_div", "bbppll_sel",    CLK_SET_RATE_PARENT, 0x4a0, 8,  6, 15,},
+	{ HI6220_CS_DAPB,     "cs_dapb",     "picophy_src",   CLK_SET_RATE_PARENT, 0x4a0, 24, 2, 31,},
+	{ HI6220_CS_ATB_DIV,  "cs_atb_div",  "cs_atb_syspll", CLK_SET_RATE_PARENT, 0x4a4, 0,  4, 7, },
+};
+
+static void __init hi6220_clk_sys_init(struct device_node *np)
+{
+	struct hisi_clock_data *clk_data;
+
+	clk_data = hisi_clk_init(np, HI6220_SYS_NR_CLKS);
+	if (!clk_data)
+		return;
+
+	hisi_clk_register_gate_sep(hi6220_separated_gate_clks_sys,
+			ARRAY_SIZE(hi6220_separated_gate_clks_sys), clk_data);
+
+	hisi_clk_register_mux(hi6220_mux_clks_sys,
+			ARRAY_SIZE(hi6220_mux_clks_sys), clk_data);
+
+	hi6220_clk_register_divider(hi6220_div_clks_sys,
+			ARRAY_SIZE(hi6220_div_clks_sys), clk_data);
+
+	if (!clk_data_ao)
+		return;
+
+	/* enable high speed clock on UART1 mux */
+	clk_set_parent(clk_data->clk_data.clks[HI6220_UART1_SRC],
+			clk_data_ao->clk_data.clks[HI6220_150M]);
+}
+CLK_OF_DECLARE(hi6220_clk_sys, "hisilicon,hi6220-sysctrl", hi6220_clk_sys_init);
+
+
+/* clocks in media controller */
+static const char *clk_1000_1200_src[] __initconst = { "pll_gpu_gate", "media_syspll_src", };
+static const char *clk_1440_1200_src[] __initconst = { "media_syspll_src", "media_pll_src", };
+static const char *clk_1000_1440_src[] __initconst = { "pll_gpu_gate", "media_pll_src", };
+
+static struct hisi_gate_clock hi6220_separated_gate_clks_media[] __initdata = {
+	{ HI6220_DSI_PCLK,       "dsi_pclk",         "vpucodec",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 0,  0, },
+	{ HI6220_G3D_PCLK,       "g3d_pclk",         "vpucodec",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 1,  0, },
+	{ HI6220_ACLK_CODEC_VPU, "aclk_codec_vpu",   "ade_core_src",  CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 3,  0, },
+	{ HI6220_ISP_SCLK,       "isp_sclk",         "isp_sclk_src",  CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 5,  0, },
+	{ HI6220_ADE_CORE,	 "ade_core",	     "ade_core_src",  CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 6,  0, },
+	{ HI6220_MED_MMU,        "media_mmu",        "mmu_clk",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 8,  0, },
+	{ HI6220_CFG_CSI4PHY,    "cfg_csi4phy",      "clk_tcxo",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 9,  0, },
+	{ HI6220_CFG_CSI2PHY,    "cfg_csi2phy",      "clk_tcxo",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 10, 0, },
+	{ HI6220_ISP_SCLK_GATE,  "isp_sclk_gate",    "media_pll_src", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 11, 0, },
+	{ HI6220_ISP_SCLK_GATE1, "isp_sclk_gate1",   "media_pll_src", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 12, 0, },
+	{ HI6220_ADE_CORE_GATE,  "ade_core_gate",    "media_pll_src", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 14, 0, },
+	{ HI6220_CODEC_VPU_GATE, "codec_vpu_gate",   "clk_1000_1440", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 15, 0, },
+	{ HI6220_MED_SYSPLL,     "media_syspll_src", "media_syspll",  CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 17, 0, },
+};
+
+static struct hisi_mux_clock hi6220_mux_clks_media[] __initdata = {
+	{ HI6220_1440_1200, "clk_1440_1200", clk_1440_1200_src, ARRAY_SIZE(clk_1440_1200_src), CLK_SET_RATE_PARENT, 0x51c, 0, 1, 0, },
+	{ HI6220_1000_1200, "clk_1000_1200", clk_1000_1200_src, ARRAY_SIZE(clk_1000_1200_src), CLK_SET_RATE_PARENT, 0x51c, 1, 1, 0, },
+	{ HI6220_1000_1440, "clk_1000_1440", clk_1000_1440_src, ARRAY_SIZE(clk_1000_1440_src), CLK_SET_RATE_PARENT, 0x51c, 6, 1, 0, },
+};
+
+static struct hi6220_divider_clock hi6220_div_clks_media[] __initdata = {
+	{ HI6220_CODEC_JPEG,    "codec_jpeg_aclk", "media_pll_src",  CLK_SET_RATE_PARENT, 0xcbc, 0,  4, 23, },
+	{ HI6220_ISP_SCLK_SRC,  "isp_sclk_src",    "isp_sclk_gate",  CLK_SET_RATE_PARENT, 0xcbc, 8,  4, 15, },
+	{ HI6220_ISP_SCLK1,     "isp_sclk1",       "isp_sclk_gate1", CLK_SET_RATE_PARENT, 0xcbc, 24, 4, 31, },
+	{ HI6220_ADE_CORE_SRC,  "ade_core_src",    "ade_core_gate",  CLK_SET_RATE_PARENT, 0xcc0, 16, 3, 23, },
+	{ HI6220_ADE_PIX_SRC,   "ade_pix_src",     "clk_1440_1200",  CLK_SET_RATE_PARENT, 0xcc0, 24, 6, 31, },
+	{ HI6220_G3D_CLK,       "g3d_clk",         "clk_1000_1200",  CLK_SET_RATE_PARENT, 0xcc4, 8,  4, 15, },
+	{ HI6220_CODEC_VPU_SRC, "codec_vpu_src",   "codec_vpu_gate", CLK_SET_RATE_PARENT, 0xcc4, 24, 6, 31, },
+};
+
+static void __init hi6220_clk_media_init(struct device_node *np)
+{
+	struct hisi_clock_data *clk_data;
+
+	clk_data = hisi_clk_init(np, HI6220_MEDIA_NR_CLKS);
+	if (!clk_data)
+		return;
+
+	hisi_clk_register_gate_sep(hi6220_separated_gate_clks_media,
+				ARRAY_SIZE(hi6220_separated_gate_clks_media), clk_data);
+
+	hisi_clk_register_mux(hi6220_mux_clks_media,
+				ARRAY_SIZE(hi6220_mux_clks_media), clk_data);
+
+	hi6220_clk_register_divider(hi6220_div_clks_media,
+				ARRAY_SIZE(hi6220_div_clks_media), clk_data);
+}
+CLK_OF_DECLARE(hi6220_clk_media, "hisilicon,hi6220-mediactrl", hi6220_clk_media_init);
+
+
+/* clocks in pmctrl */
+static struct hisi_gate_clock hi6220_gate_clks_power[] __initdata = {
+	{ HI6220_PLL_GPU_GATE,   "pll_gpu_gate",   "gpupll",    CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x8,  0,  0, },
+	{ HI6220_PLL1_DDR_GATE,  "pll1_ddr_gate",  "ddrpll1",   CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x10, 0,  0, },
+	{ HI6220_PLL_DDR_GATE,   "pll_ddr_gate",   "ddrpll0",   CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x18, 0,  0, },
+	{ HI6220_PLL_MEDIA_GATE, "pll_media_gate", "media_pll", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x38, 0,  0, },
+	{ HI6220_PLL0_BBP_GATE,  "pll0_bbp_gate",  "bbppll0",   CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x48, 0,  0, },
+};
+
+static struct hi6220_divider_clock hi6220_div_clks_power[] __initdata = {
+	{ HI6220_DDRC_SRC,  "ddrc_src",  "ddr_sel_src", CLK_SET_RATE_PARENT, 0x5a8, 0, 4, 0, },
+	{ HI6220_DDRC_AXI1, "ddrc_axi1", "ddrc_src",    CLK_SET_RATE_PARENT, 0x5a8, 8, 2, 0, },
+};
+
+static void __init hi6220_clk_power_init(struct device_node *np)
+{
+	struct hisi_clock_data *clk_data;
+
+	clk_data = hisi_clk_init(np, HI6220_POWER_NR_CLKS);
+	if (!clk_data)
+		return;
+
+	hisi_clk_register_gate(hi6220_gate_clks_power,
+				ARRAY_SIZE(hi6220_gate_clks_power), clk_data);
+
+	hi6220_clk_register_divider(hi6220_div_clks_power,
+				ARRAY_SIZE(hi6220_div_clks_power), clk_data);
+}
+CLK_OF_DECLARE(hi6220_clk_power, "hisilicon,hi6220-pmctrl", hi6220_clk_power_init);
diff --git a/drivers/clk/hisilicon/clk.c b/drivers/clk/hisilicon/clk.c
index a078e84..5d2305c 100644
--- a/drivers/clk/hisilicon/clk.c
+++ b/drivers/clk/hisilicon/clk.c
@@ -232,3 +232,32 @@ void __init hisi_clk_register_gate_sep(struct hisi_gate_clock *clks,
 		data->clk_data.clks[clks[i].id] = clk;
 	}
 }
+
+void __init hi6220_clk_register_divider(struct hi6220_divider_clock *clks,
+					  int nums, struct hisi_clock_data *data)
+{
+	struct clk *clk;
+	void __iomem *base = data->base;
+	int i;
+
+	for (i = 0; i < nums; i++) {
+		clk = hi6220_register_clkdiv(NULL, clks[i].name,
+						clks[i].parent_name,
+						clks[i].flags,
+						base + clks[i].offset,
+						clks[i].shift,
+						clks[i].width,
+						clks[i].mask_bit,
+						&hisi_clk_lock);
+		if (IS_ERR(clk)) {
+			pr_err("%s: failed to register clock %s\n",
+			       __func__, clks[i].name);
+			continue;
+		}
+
+		if (clks[i].alias)
+			clk_register_clkdev(clk, clks[i].alias, NULL);
+
+		data->clk_data.clks[clks[i].id] = clk;
+	}
+}
diff --git a/drivers/clk/hisilicon/clk.h b/drivers/clk/hisilicon/clk.h
index 31083ff..462a570 100644
--- a/drivers/clk/hisilicon/clk.h
+++ b/drivers/clk/hisilicon/clk.h
@@ -79,6 +79,18 @@ struct hisi_divider_clock {
 	const char		*alias;
 };
 
+struct hi6220_divider_clock {
+	unsigned int		id;
+	const char		*name;
+	const char		*parent_name;
+	unsigned long		flags;
+	unsigned long		offset;
+	u8			shift;
+	u8			width;
+	u32			mask_bit;
+	const char		*alias;
+};
+
 struct hisi_gate_clock {
 	unsigned int		id;
 	const char		*name;
@@ -94,6 +106,9 @@ struct clk *hisi_register_clkgate_sep(struct device *, const char *,
 				const char *, unsigned long,
 				void __iomem *, u8,
 				u8, spinlock_t *);
+struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
+	const char *parent_name, unsigned long flags, void __iomem *reg,
+	u8 shift, u8 width, u32 mask_bit, spinlock_t *lock);
 
 struct hisi_clock_data __init *hisi_clk_init(struct device_node *, int);
 void __init hisi_clk_register_fixed_rate(struct hisi_fixed_rate_clock *,
@@ -108,4 +123,6 @@ void __init hisi_clk_register_gate(struct hisi_gate_clock *,
 					int, struct hisi_clock_data *);
 void __init hisi_clk_register_gate_sep(struct hisi_gate_clock *,
 					int, struct hisi_clock_data *);
+void __init hi6220_clk_register_divider(struct hi6220_divider_clock *,
+					int, struct hisi_clock_data *);
 #endif	/* __HISI_CLK_H */
diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
new file mode 100644
index 0000000..9e3825b
--- /dev/null
+++ b/drivers/clk/hisilicon/clkdivider-hi6220.c
@@ -0,0 +1,273 @@
+/*
+ * Hisilicon hi6220 SoC divider clock driver
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * Author: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/err.h>
+
+#define div_mask(width)	((1 << (width)) - 1)
+
+/*
+ * The reverse of DIV_ROUND_UP: The maximum number which
+ * divided by m is r
+ */
+#define MULT_ROUND_UP(r, m) ((r) * (m) + (m) - 1)
+
+/**
+ * struct hi6220_clk_divider - divider clock for hi6220
+ *
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register containing divider
+ * @shift:	shift to the divider bit field
+ * @width:	width of the divider bit field
+ * @mask:	mask for setting divider rate
+ * @table:	the div table that the divider supports
+ * @lock:	register lock
+ */
+struct hi6220_clk_divider {
+	struct clk_hw	hw;
+	void __iomem	*reg;
+	u8		shift;
+	u8		width;
+	u32		mask;
+	const struct clk_div_table *table;
+	spinlock_t	*lock;
+};
+
+#define to_hi6220_clk_divider(_hw)	\
+	container_of(_hw, struct hi6220_clk_divider, hw)
+
+static unsigned int hi6220_get_table_maxdiv(const struct clk_div_table *table)
+{
+	unsigned int maxdiv = 0;
+	const struct clk_div_table *clkt;
+
+	for (clkt = table; clkt->div; clkt++)
+		if (clkt->div > maxdiv)
+			maxdiv = clkt->div;
+	return maxdiv;
+}
+
+static unsigned int hi6220_get_table_div(const struct clk_div_table *table,
+					unsigned int val)
+{
+	const struct clk_div_table *clkt;
+
+	for (clkt = table; clkt->div; clkt++)
+		if (clkt->val == val)
+			return clkt->div;
+
+	return 0;
+}
+
+static unsigned int hi6220_get_table_val(const struct clk_div_table *table,
+					unsigned int div)
+{
+	const struct clk_div_table *clkt;
+
+	for (clkt = table; clkt->div; clkt++)
+		if (clkt->div == div)
+			return clkt->val;
+	return 0;
+}
+
+static unsigned long hi6220_clkdiv_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	unsigned int div, val;
+	struct hi6220_clk_divider *dclk = to_hi6220_clk_divider(hw);
+
+	val = readl_relaxed(dclk->reg) >> dclk->shift;
+	val &= div_mask(dclk->width);
+
+	div = hi6220_get_table_div(dclk->table, val);
+	if (!div) {
+		pr_warn("%s: Invalid divisor for clock %s\n", __func__,
+			   __clk_get_name(hw->clk));
+		return parent_rate;
+	}
+
+	return parent_rate / div;
+}
+
+static bool hi6220_is_valid_div(const struct clk_div_table *table,
+				unsigned int div)
+{
+	const struct clk_div_table *clkt;
+
+	for (clkt = table; clkt->div; clkt++)
+		if (clkt->div == div)
+			return true;
+	return false;
+}
+
+static int hi6220_clkdiv_bestdiv(struct clk_hw *hw, unsigned long rate,
+				 unsigned long *best_parent_rate)
+{
+	unsigned int i, bestdiv = 0;
+	unsigned long parent_rate, best = 0, now, maxdiv;
+
+	struct hi6220_clk_divider *dclk = to_hi6220_clk_divider(hw);
+	struct clk *clk_parent = __clk_get_parent(hw->clk);
+
+	maxdiv = hi6220_get_table_maxdiv(dclk->table);
+
+	if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT)) {
+		parent_rate = *best_parent_rate;
+		bestdiv = DIV_ROUND_UP(parent_rate, rate);
+		bestdiv = (bestdiv == 0) ? 1 : bestdiv;
+		bestdiv = (bestdiv > maxdiv) ? maxdiv : bestdiv;
+		return bestdiv;
+	}
+
+	/*
+	 * The maximum divider we can use without overflowing
+	 * unsigned long in rate * i below
+	 */
+	maxdiv = min(ULONG_MAX / rate, maxdiv);
+
+	for (i = 1; i <= maxdiv; i++) {
+		if (!hi6220_is_valid_div(dclk->table, i))
+			continue;
+		parent_rate = __clk_round_rate(clk_parent,
+					MULT_ROUND_UP(rate, i));
+		now = parent_rate / i;
+		if (now <= rate && now > best) {
+			bestdiv = i;
+			best = now;
+			*best_parent_rate = parent_rate;
+		}
+	}
+
+	if (!bestdiv) {
+		bestdiv = hi6220_get_table_maxdiv(dclk->table);
+		*best_parent_rate = __clk_round_rate(clk_parent, 1);
+	}
+
+	return bestdiv;
+}
+
+static long hi6220_clkdiv_round_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long *prate)
+{
+	int div;
+
+	if (!rate)
+		rate = 1;
+
+	div = hi6220_clkdiv_bestdiv(hw, rate, prate);
+
+	return *prate / div;
+}
+
+static int hi6220_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long parent_rate)
+{
+	unsigned int div, value;
+	unsigned long flags = 0;
+	u32 data;
+	struct hi6220_clk_divider *dclk = to_hi6220_clk_divider(hw);
+
+	div = parent_rate / rate;
+
+	if (!hi6220_is_valid_div(dclk->table, div))
+		return -EINVAL;
+
+	value = hi6220_get_table_val(dclk->table, div);
+
+	if (value > div_mask(dclk->width))
+		value = div_mask(dclk->width);
+
+	if (dclk->lock)
+		spin_lock_irqsave(dclk->lock, flags);
+
+	data = readl_relaxed(dclk->reg);
+	data &= ~(div_mask(dclk->width) << dclk->shift);
+	data |= value << dclk->shift;
+	data |= dclk->mask;
+
+	writel_relaxed(data, dclk->reg);
+
+	if (dclk->lock)
+		spin_unlock_irqrestore(dclk->lock, flags);
+
+	return 0;
+}
+
+static struct clk_ops hi6220_clkdiv_ops = {
+	.recalc_rate = hi6220_clkdiv_recalc_rate,
+	.round_rate = hi6220_clkdiv_round_rate,
+	.set_rate = hi6220_clkdiv_set_rate,
+};
+
+struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
+	const char *parent_name, unsigned long flags, void __iomem *reg,
+	u8 shift, u8 width, u32 mask_bit, spinlock_t *lock)
+{
+	struct hi6220_clk_divider *div;
+	struct clk *clk;
+	struct clk_init_data init;
+	struct clk_div_table *table;
+	u32 max_div, min_div;
+	int i;
+
+	/* allocate the divider */
+	div = kzalloc(sizeof(struct hi6220_clk_divider), GFP_KERNEL);
+	if (!div) {
+		pr_err("%s: could not allocate divider clk\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/* Init the divider table */
+	max_div = div_mask(width) + 1;
+	min_div = 1;
+
+	table = kzalloc(sizeof(struct clk_div_table) * (max_div + 1), GFP_KERNEL);
+	if (!table) {
+		kfree(div);
+		pr_err("%s: failed to alloc divider table!\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0; i < max_div; i++) {
+		table[i].div = min_div + i;
+		table[i].val = table[i].div - 1;
+	}
+
+	init.name = name;
+	init.ops = &hi6220_clkdiv_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;
+
+	/* struct hi6220_clk_divider assignments */
+	div->reg = reg;
+	div->shift = shift;
+	div->width = width;
+	div->mask = mask_bit ? BIT(mask_bit) : 0;
+	div->lock = lock;
+	div->hw.init = &init;
+	div->table = table;
+
+	/* register the clock */
+	clk = clk_register(dev, &div->hw);
+
+	if (IS_ERR(clk)) {
+		kfree(table);
+		kfree(div);
+	}
+
+	return clk;
+}
diff --git a/include/dt-bindings/clock/hi6220-clock.h b/include/dt-bindings/clock/hi6220-clock.h
new file mode 100644
index 0000000..70ee383
--- /dev/null
+++ b/include/dt-bindings/clock/hi6220-clock.h
@@ -0,0 +1,173 @@
+/*
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * Author: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_HI6220_H
+#define __DT_BINDINGS_CLOCK_HI6220_H
+
+/* clk in Hi6220 AO (always on) controller */
+#define HI6220_NONE_CLOCK	0
+
+/* fixed rate clocks */
+#define HI6220_REF32K		1
+#define HI6220_CLK_TCXO		2
+#define HI6220_MMC1_PAD		3
+#define HI6220_MMC2_PAD		4
+#define HI6220_MMC0_PAD		5
+#define HI6220_PLL_BBP		6
+#define HI6220_PLL_GPU		7
+#define HI6220_PLL1_DDR		8
+#define HI6220_PLL_SYS		9
+#define HI6220_PLL_SYS_MEDIA	10
+#define HI6220_DDR_SRC		11
+#define HI6220_PLL_MEDIA	12
+#define HI6220_PLL_DDR		13
+
+/* fixed factor clocks */
+#define HI6220_300M		14
+#define HI6220_150M		15
+#define HI6220_PICOPHY_SRC	16
+#define HI6220_MMC0_SRC_SEL	17
+#define HI6220_MMC1_SRC_SEL	18
+#define HI6220_MMC2_SRC_SEL	19
+#define HI6220_VPU_CODEC	20
+#define HI6220_MMC0_SMP		21
+#define HI6220_MMC1_SMP		22
+#define HI6220_MMC2_SMP		23
+
+/* gate clocks */
+#define HI6220_WDT0_PCLK	24
+#define HI6220_WDT1_PCLK	25
+#define HI6220_WDT2_PCLK	26
+#define HI6220_TIMER0_PCLK	27
+#define HI6220_TIMER1_PCLK	28
+#define HI6220_TIMER2_PCLK	29
+#define HI6220_TIMER3_PCLK	30
+#define HI6220_TIMER4_PCLK	31
+#define HI6220_TIMER5_PCLK	32
+#define HI6220_TIMER6_PCLK	33
+#define HI6220_TIMER7_PCLK	34
+#define HI6220_TIMER8_PCLK	35
+#define HI6220_UART0_PCLK	36
+
+#define HI6220_AO_NR_CLKS	37
+
+/* clk in Hi6220 systrl */
+/* gate clock */
+#define HI6220_MMC0_CLK		1
+#define HI6220_MMC0_CIUCLK	2
+#define HI6220_MMC1_CLK		3
+#define HI6220_MMC1_CIUCLK	4
+#define HI6220_MMC2_CLK		5
+#define HI6220_MMC2_CIUCLK	6
+#define HI6220_USBOTG_HCLK	7
+#define HI6220_CLK_PICOPHY	8
+#define HI6220_HIFI		9
+#define HI6220_DACODEC_PCLK	10
+#define HI6220_EDMAC_ACLK	11
+#define HI6220_CS_ATB		12
+#define HI6220_I2C0_CLK		13
+#define HI6220_I2C1_CLK		14
+#define HI6220_I2C2_CLK		15
+#define HI6220_I2C3_CLK		16
+#define HI6220_UART1_PCLK	17
+#define HI6220_UART2_PCLK	18
+#define HI6220_UART3_PCLK	19
+#define HI6220_UART4_PCLK	20
+#define HI6220_SPI_CLK		21
+#define HI6220_TSENSOR_CLK	22
+#define HI6220_MMU_CLK		23
+#define HI6220_HIFI_SEL		24
+#define HI6220_MMC0_SYSPLL	25
+#define HI6220_MMC1_SYSPLL	26
+#define HI6220_MMC2_SYSPLL	27
+#define HI6220_MMC0_SEL		28
+#define HI6220_MMC1_SEL		29
+#define HI6220_BBPPLL_SEL	30
+#define HI6220_MEDIA_PLL_SRC	31
+#define HI6220_MMC2_SEL		32
+#define HI6220_CS_ATB_SYSPLL	33
+
+/* mux clocks */
+#define HI6220_MMC0_SRC		34
+#define HI6220_MMC0_SMP_IN	35
+#define HI6220_MMC1_SRC		36
+#define HI6220_MMC1_SMP_IN	37
+#define HI6220_MMC2_SRC		38
+#define HI6220_MMC2_SMP_IN	39
+#define HI6220_HIFI_SRC		40
+#define HI6220_UART1_SRC	41
+#define HI6220_UART2_SRC	42
+#define HI6220_UART3_SRC	43
+#define HI6220_UART4_SRC	44
+#define HI6220_MMC0_MUX0	45
+#define HI6220_MMC1_MUX0	46
+#define HI6220_MMC2_MUX0	47
+#define HI6220_MMC0_MUX1	48
+#define HI6220_MMC1_MUX1	49
+#define HI6220_MMC2_MUX1	50
+
+/* divider clocks */
+#define HI6220_CLK_BUS		51
+#define HI6220_MMC0_DIV		52
+#define HI6220_MMC1_DIV		53
+#define HI6220_MMC2_DIV		54
+#define HI6220_HIFI_DIV		55
+#define HI6220_BBPPLL0_DIV	56
+#define HI6220_CS_DAPB		57
+#define HI6220_CS_ATB_DIV	58
+
+#define HI6220_SYS_NR_CLKS	59
+
+/* clk in Hi6220 media controller */
+/* gate clocks */
+#define HI6220_DSI_PCLK		1
+#define HI6220_G3D_PCLK		2
+#define HI6220_ACLK_CODEC_VPU	3
+#define HI6220_ISP_SCLK		4
+#define HI6220_ADE_CORE		5
+#define HI6220_MED_MMU		6
+#define HI6220_CFG_CSI4PHY	7
+#define HI6220_CFG_CSI2PHY	8
+#define HI6220_ISP_SCLK_GATE	9
+#define HI6220_ISP_SCLK_GATE1	10
+#define HI6220_ADE_CORE_GATE	11
+#define HI6220_CODEC_VPU_GATE	12
+#define HI6220_MED_SYSPLL	13
+
+/* mux clocks */
+#define HI6220_1440_1200	14
+#define HI6220_1000_1200	15
+#define HI6220_1000_1440	16
+
+/* divider clocks */
+#define HI6220_CODEC_JPEG	17
+#define HI6220_ISP_SCLK_SRC	18
+#define HI6220_ISP_SCLK1	19
+#define HI6220_ADE_CORE_SRC	20
+#define HI6220_ADE_PIX_SRC	21
+#define HI6220_G3D_CLK		22
+#define HI6220_CODEC_VPU_SRC	23
+
+#define HI6220_MEDIA_NR_CLKS	24
+
+/* clk in Hi6220 power controller */
+/* gate clocks */
+#define HI6220_PLL_GPU_GATE	1
+#define HI6220_PLL1_DDR_GATE	2
+#define HI6220_PLL_DDR_GATE	3
+#define HI6220_PLL_MEDIA_GATE	4
+#define HI6220_PLL0_BBP_GATE	5
+
+/* divider clocks */
+#define HI6220_DDRC_SRC		6
+#define HI6220_DDRC_AXI1	7
+
+#define HI6220_POWER_NR_CLKS	8
+#endif
-- 
1.9.1

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

Re: [PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC

[Stephen Boyd]

On 05/05, Bintian Wang wrote:
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9897f35..935c44b 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -152,6 +152,8 @@ config COMMON_CLK_CDCE706
>  
>  source "drivers/clk/qcom/Kconfig"
>  
> +source "drivers/clk/hisilicon/Kconfig"
> +

Please move this above qcom to maintain alphabet sort.

>  endmenu
>  
>  source "drivers/clk/bcm/Kconfig"
> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> new file mode 100644
> index 0000000..8034739
> --- /dev/null
> +++ b/drivers/clk/hisilicon/Kconfig
> @@ -0,0 +1,6 @@
> +config COMMON_CLK_HI6220
> +	bool "Hi6220 Clock Driver"
> +	depends on OF && ARCH_HISI
> +	default y

Can this be

	depends on ARCH_HISI || COMPILE_TEST
	default ARCH_HISI

instead? I'd like to increase build coverage.

> +	help
> +	  Build the Hisilicon Hi6220 clock driver based on the common clock framework.
> diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c
> new file mode 100644
> index 0000000..91b1cd7
> --- /dev/null
> +++ b/drivers/clk/hisilicon/clk-hi6220.c
> @@ -0,0 +1,292 @@
> +/*
> + * Hisilicon Hi6220 clock driver
> + *
> + * Copyright (c) 2015 Hisilicon Limited.
> + *
> + * Author: Bintian Wang <bintian.wang@huawei.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>

Do we need to include linux/clk.h? I don't see any consumer
usage here.

> +
> +#include <dt-bindings/clock/hi6220-clock.h>
> +
> +#include "clk.h"
> +
> diff --git a/drivers/clk/hisilicon/clk.c b/drivers/clk/hisilicon/clk.c
> index a078e84..5d2305c 100644
> --- a/drivers/clk/hisilicon/clk.c
> +++ b/drivers/clk/hisilicon/clk.c
> @@ -108,4 +123,6 @@ void __init hisi_clk_register_gate(struct hisi_gate_clock *,
>  					int, struct hisi_clock_data *);
>  void __init hisi_clk_register_gate_sep(struct hisi_gate_clock *,
>  					int, struct hisi_clock_data *);
> +void __init hi6220_clk_register_divider(struct hi6220_divider_clock *,
> +					int, struct hisi_clock_data *);

__init markings on function prototypes are useless. Please remove them.

>  #endif	/* __HISI_CLK_H */
> diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
> +
> +/**
> + * struct hi6220_clk_divider - divider clock for hi6220
> + *
> + * @hw:		handle between common and hardware-specific interfaces
> + * @reg:	register containing divider
> + * @shift:	shift to the divider bit field
> + * @width:	width of the divider bit field
> + * @mask:	mask for setting divider rate
> + * @table:	the div table that the divider supports
> + * @lock:	register lock
> + */
> +struct hi6220_clk_divider {
> +	struct clk_hw	hw;
> +	void __iomem	*reg;
> +	u8		shift;
> +	u8		width;
> +	u32		mask;
> +	const struct clk_div_table *table;
> +	spinlock_t	*lock;
> +};

The clk-divider.c code has been made "reusable". Can you please
try to use the functions that it now exposes instead of
copy/pasting it and modifying it to suit your needs? A lot of
this code looks the same.

> +
> +#define to_hi6220_clk_divider(_hw)	\
[..]
> +
> +static struct clk_ops hi6220_clkdiv_ops = {

const?

> +	.recalc_rate = hi6220_clkdiv_recalc_rate,
> +	.round_rate = hi6220_clkdiv_round_rate,
> +	.set_rate = hi6220_clkdiv_set_rate,
> +};
> +
> +struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
> +	const char *parent_name, unsigned long flags, void __iomem *reg,
> +	u8 shift, u8 width, u32 mask_bit, spinlock_t *lock)
> +{
> +	struct hi6220_clk_divider *div;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +	struct clk_div_table *table;
> +	u32 max_div, min_div;
> +	int i;
> +
> +	/* allocate the divider */
> +	div = kzalloc(sizeof(struct hi6220_clk_divider), GFP_KERNEL);
> +	if (!div) {
> +		pr_err("%s: could not allocate divider clk\n", __func__);

Useless error message on allocation failure. Please run
checkpatch. I've sent a patch to remove these from basic clock
types.

> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Init the divider table */
> +	max_div = div_mask(width) + 1;
> +	min_div = 1;
> +
> +	table = kzalloc(sizeof(struct clk_div_table) * (max_div + 1), GFP_KERNEL);
> +	if (!table) {
> +		kfree(div);
> +		pr_err("%s: failed to alloc divider table!\n", __func__);

ditto.

> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	for (i = 0; i < max_div; i++) {
> +		table[i].div = min_div + i;
> +		table[i].val = table[i].div - 1;
> +	}
> +
> +	init.name = name;
> +	init.ops = &hi6220_clkdiv_ops;
> +	init.flags = flags | CLK_IS_BASIC;

It's basic?

> +	init.parent_names = parent_name ? &parent_name : NULL;
> +	init.num_parents = parent_name ? 1 : 0;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC

[Bintian]

Hello Stephen,

On 2015/5/15 8:25, Stephen Boyd wrote:
> On 05/05, Bintian Wang wrote:
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 9897f35..935c44b 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -152,6 +152,8 @@ config COMMON_CLK_CDCE706
>>
>>   source "drivers/clk/qcom/Kconfig"
>>
>> +source "drivers/clk/hisilicon/Kconfig"
>> +
>
> Please move this above qcom to maintain alphabet sort.
OK, fix in next version.

>
>>   endmenu
>>
>>   source "drivers/clk/bcm/Kconfig"
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> new file mode 100644
>> index 0000000..8034739
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -0,0 +1,6 @@
>> +config COMMON_CLK_HI6220
>> +	bool "Hi6220 Clock Driver"
>> +	depends on OF && ARCH_HISI
>> +	default y
>
> Can this be
>
> 	depends on ARCH_HISI || COMPILE_TEST
> 	default ARCH_HISI
>
> instead? I'd like to increase build coverage.
No problem, will fix in next version.

>
>> +	help
>> +	  Build the Hisilicon Hi6220 clock driver based on the common clock framework.
>> diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c
>> new file mode 100644
>> index 0000000..91b1cd7
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-hi6220.c
>> @@ -0,0 +1,292 @@
>> +/*
>> + * Hisilicon Hi6220 clock driver
>> + *
>> + * Copyright (c) 2015 Hisilicon Limited.
>> + *
>> + * Author: Bintian Wang <bintian.wang@huawei.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>
> Do we need to include linux/clk.h? I don't see any consumer
> usage here.
You are right, remove in next version.

>
>> +
>> +#include <dt-bindings/clock/hi6220-clock.h>
>> +
>> +#include "clk.h"
>> +
>> diff --git a/drivers/clk/hisilicon/clk.c b/drivers/clk/hisilicon/clk.c
>> index a078e84..5d2305c 100644
>> --- a/drivers/clk/hisilicon/clk.c
>> +++ b/drivers/clk/hisilicon/clk.c
>> @@ -108,4 +123,6 @@ void __init hisi_clk_register_gate(struct hisi_gate_clock *,
>>   					int, struct hisi_clock_data *);
>>   void __init hisi_clk_register_gate_sep(struct hisi_gate_clock *,
>>   					int, struct hisi_clock_data *);
>> +void __init hi6220_clk_register_divider(struct hi6220_divider_clock *,
>> +					int, struct hisi_clock_data *);
>
> __init markings on function prototypes are useless. Please remove them.
OK

>
>>   #endif	/* __HISI_CLK_H */
>> diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
>> +
>> +/**
>> + * struct hi6220_clk_divider - divider clock for hi6220
>> + *
>> + * @hw:		handle between common and hardware-specific interfaces
>> + * @reg:	register containing divider
>> + * @shift:	shift to the divider bit field
>> + * @width:	width of the divider bit field
>> + * @mask:	mask for setting divider rate
>> + * @table:	the div table that the divider supports
>> + * @lock:	register lock
>> + */
>> +struct hi6220_clk_divider {
>> +	struct clk_hw	hw;
>> +	void __iomem	*reg;
>> +	u8		shift;
>> +	u8		width;
>> +	u32		mask;
>> +	const struct clk_div_table *table;
>> +	spinlock_t	*lock;
>> +};
>
> The clk-divider.c code has been made "reusable". Can you please
> try to use the functions that it now exposes instead of
> copy/pasting it and modifying it to suit your needs? A lot of
> this code looks the same.
In fact, I discussed this problem with Rob Herring and Mike Turquette
in the 96boards internal mail list before.

The divider in hi6220 has a mask bit to guarantee writing the correct
bits in register when setting rate, but the index of this mask bit has
no rules to get (e.g. by left shift some fixed bits), so I add this
divider clock to handle it, we can regard hi6220_clk_divider as a
special case of generic divider clock.

If I don't add this divider clock for hi6220 chip, then I should change
the core APIs "clk_register_divider" and "clk_register_divider_table",
and then many other drivers will be updated.
So I think just add this divider clock is a good solution now.

>
>> +
>> +#define to_hi6220_clk_divider(_hw)	\
> [..]
>> +
>> +static struct clk_ops hi6220_clkdiv_ops = {
>
> const?
Add in next version.

>
>> +	.recalc_rate = hi6220_clkdiv_recalc_rate,
>> +	.round_rate = hi6220_clkdiv_round_rate,
>> +	.set_rate = hi6220_clkdiv_set_rate,
>> +};
>> +
>> +struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
>> +	const char *parent_name, unsigned long flags, void __iomem *reg,
>> +	u8 shift, u8 width, u32 mask_bit, spinlock_t *lock)
>> +{
>> +	struct hi6220_clk_divider *div;
>> +	struct clk *clk;
>> +	struct clk_init_data init;
>> +	struct clk_div_table *table;
>> +	u32 max_div, min_div;
>> +	int i;
>> +
>> +	/* allocate the divider */
>> +	div = kzalloc(sizeof(struct hi6220_clk_divider), GFP_KERNEL);
>> +	if (!div) {
>> +		pr_err("%s: could not allocate divider clk\n", __func__);
>
> Useless error message on allocation failure. Please run
> checkpatch. I've sent a patch to remove these from basic clock
> types.
Remove in next version.
>
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	/* Init the divider table */
>> +	max_div = div_mask(width) + 1;
>> +	min_div = 1;
>> +
>> +	table = kzalloc(sizeof(struct clk_div_table) * (max_div + 1), GFP_KERNEL);
>> +	if (!table) {
>> +		kfree(div);
>> +		pr_err("%s: failed to alloc divider table!\n", __func__);
>
> ditto.
Will remove in next version.
>
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	for (i = 0; i < max_div; i++) {
>> +		table[i].div = min_div + i;
>> +		table[i].val = table[i].div - 1;
>> +	}
>> +
>> +	init.name = name;
>> +	init.ops = &hi6220_clkdiv_ops;
>> +	init.flags = flags | CLK_IS_BASIC;
>
> It's basic?
I rechecked this flag, it's really useless to us, so I can remove it.
But can you tell me which case I should use it?

How about just send this patch for review not the whole patch set in 
next version?

Thanks,

Bintian
>
>> +	init.parent_names = parent_name ? &parent_name : NULL;
>> +	init.num_parents = parent_name ? 1 : 0;
>

Re: [PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC

[Stephen Boyd]

On 05/15, Bintian wrote:
> On 2015/5/15 8:25, Stephen Boyd wrote:
> >On 05/05, Bintian Wang wrote:
> >>diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
> >>+
> >>+/**
> >>+ * struct hi6220_clk_divider - divider clock for hi6220
> >>+ *
> >>+ * @hw:		handle between common and hardware-specific interfaces
> >>+ * @reg:	register containing divider
> >>+ * @shift:	shift to the divider bit field
> >>+ * @width:	width of the divider bit field
> >>+ * @mask:	mask for setting divider rate
> >>+ * @table:	the div table that the divider supports
> >>+ * @lock:	register lock
> >>+ */
> >>+struct hi6220_clk_divider {
> >>+	struct clk_hw	hw;
> >>+	void __iomem	*reg;
> >>+	u8		shift;
> >>+	u8		width;
> >>+	u32		mask;
> >>+	const struct clk_div_table *table;
> >>+	spinlock_t	*lock;
> >>+};
> >
> >The clk-divider.c code has been made "reusable". Can you please
> >try to use the functions that it now exposes instead of
> >copy/pasting it and modifying it to suit your needs? A lot of
> >this code looks the same.
> In fact, I discussed this problem with Rob Herring and Mike Turquette
> in the 96boards internal mail list before.
> 
> The divider in hi6220 has a mask bit to guarantee writing the correct
> bits in register when setting rate, but the index of this mask bit has
> no rules to get (e.g. by left shift some fixed bits), so I add this
> divider clock to handle it, we can regard hi6220_clk_divider as a
> special case of generic divider clock.
> 
> If I don't add this divider clock for hi6220 chip, then I should change
> the core APIs "clk_register_divider" and "clk_register_divider_table",
> and then many other drivers will be updated.
> So I think just add this divider clock is a good solution now.

I think you missed my point. I didn't suggest using
clk_register_divider or clk_register_divider_table(). I'm
suggesting to use 

unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
                unsigned int val, const struct clk_div_table *table,
                unsigned long flags);
long divider_round_rate(struct clk_hw *hw, unsigned long rate,
                unsigned long *prate, const struct clk_div_table *table,
                u8 width, unsigned long flags);
int divider_get_val(unsigned long rate, unsigned long parent_rate,
                const struct clk_div_table *table, u8 width,
                unsigned long flags);

> >>+		return ERR_PTR(-ENOMEM);
> >>+	}
> >>+
> >>+	for (i = 0; i < max_div; i++) {
> >>+		table[i].div = min_div + i;
> >>+		table[i].val = table[i].div - 1;
> >>+	}
> >>+
> >>+	init.name = name;
> >>+	init.ops = &hi6220_clkdiv_ops;
> >>+	init.flags = flags | CLK_IS_BASIC;
> >
> >It's basic?
> I rechecked this flag, it's really useless to us, so I can remove it.
> But can you tell me which case I should use it?

I think the basic flag is there for drivers that want to know what type
of clock they're dealing with when all they have is the struct clk_hw
pointer. I like to discourage use of this flag in hopes of deleting
it someday.

> 
> How about just send this patch for review not the whole patch set in
> next version?
> 

Yes a single patch is fine. I take it you want the patch to go
through arm-soc with some Ack from us?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC

[Brent Wang]

Hello  Stephen,

2015-05-16 3:30 GMT+08:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 05/15, Bintian wrote:
>> On 2015/5/15 8:25, Stephen Boyd wrote:
>> >On 05/05, Bintian Wang wrote:
>> >>diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
>> >>+
>> >>+/**
>> >>+ * struct hi6220_clk_divider - divider clock for hi6220
>> >>+ *
>> >>+ * @hw:            handle between common and hardware-specific interfaces
>> >>+ * @reg:   register containing divider
>> >>+ * @shift: shift to the divider bit field
>> >>+ * @width: width of the divider bit field
>> >>+ * @mask:  mask for setting divider rate
>> >>+ * @table: the div table that the divider supports
>> >>+ * @lock:  register lock
>> >>+ */
>> >>+struct hi6220_clk_divider {
>> >>+   struct clk_hw   hw;
>> >>+   void __iomem    *reg;
>> >>+   u8              shift;
>> >>+   u8              width;
>> >>+   u32             mask;
>> >>+   const struct clk_div_table *table;
>> >>+   spinlock_t      *lock;
>> >>+};
>> >
>> >The clk-divider.c code has been made "reusable". Can you please
>> >try to use the functions that it now exposes instead of
>> >copy/pasting it and modifying it to suit your needs? A lot of
>> >this code looks the same.
>> In fact, I discussed this problem with Rob Herring and Mike Turquette
>> in the 96boards internal mail list before.
>>
>> The divider in hi6220 has a mask bit to guarantee writing the correct
>> bits in register when setting rate, but the index of this mask bit has
>> no rules to get (e.g. by left shift some fixed bits), so I add this
>> divider clock to handle it, we can regard hi6220_clk_divider as a
>> special case of generic divider clock.
>>
>> If I don't add this divider clock for hi6220 chip, then I should change
>> the core APIs "clk_register_divider" and "clk_register_divider_table",
>> and then many other drivers will be updated.
>> So I think just add this divider clock is a good solution now.
>
> I think you missed my point. I didn't suggest using
> clk_register_divider or clk_register_divider_table(). I'm
> suggesting to use
>
> unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
>                 unsigned int val, const struct clk_div_table *table,
>                 unsigned long flags);
> long divider_round_rate(struct clk_hw *hw, unsigned long rate,
>                 unsigned long *prate, const struct clk_div_table *table,
>                 u8 width, unsigned long flags);
> int divider_get_val(unsigned long rate, unsigned long parent_rate,
>                 const struct clk_div_table *table, u8 width,
>                 unsigned long flags);
Got it and I will prepare next version soon.

>
>> >>+           return ERR_PTR(-ENOMEM);
>> >>+   }
>> >>+
>> >>+   for (i = 0; i < max_div; i++) {
>> >>+           table[i].div = min_div + i;
>> >>+           table[i].val = table[i].div - 1;
>> >>+   }
>> >>+
>> >>+   init.name = name;
>> >>+   init.ops = &hi6220_clkdiv_ops;
>> >>+   init.flags = flags | CLK_IS_BASIC;
>> >
>> >It's basic?
>> I rechecked this flag, it's really useless to us, so I can remove it.
>> But can you tell me which case I should use it?
>
> I think the basic flag is there for drivers that want to know what type
> of clock they're dealing with when all they have is the struct clk_hw
> pointer. I like to discourage use of this flag in hopes of deleting
> it someday.
>
>>
>> How about just send this patch for review not the whole patch set in
>> next version?
>>
>
> Yes a single patch is fine. I take it you want the patch to go
> through arm-soc with some Ack from us?
Yes, exactly.
The dts file includes the clock head file,  this patch goes through
arm-soc is a good choice.

Thanks,

Bintian
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC

[Stephen Boyd]

On 05/15/15 19:54, Brent Wang wrote:
>
>>> How about just send this patch for review not the whole patch set in
>>> next version?
>>>
>> Yes a single patch is fine. I take it you want the patch to go
>> through arm-soc with some Ack from us?
> Yes, exactly.
> The dts file includes the clock head file,  this patch goes through
> arm-soc is a good choice.

One way to avoid that problem is to split the clock header file into its
own patch and then duplicate that patch in two trees, one that goes
through arm-soc and one that goes through clk.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC

[Bintian]

Hello Stephen,

On 2015/5/20 4:35, Stephen Boyd wrote:
> On 05/15/15 19:54, Brent Wang wrote:
>>
>>>> How about just send this patch for review not the whole patch set in
>>>> next version?
>>>>
>>> Yes a single patch is fine. I take it you want the patch to go
>>> through arm-soc with some Ack from us?
>> Yes, exactly.
>> The dts file includes the clock head file,  this patch goes through
>> arm-soc is a good choice.
>
> One way to avoid that problem is to split the clock header file into its
> own patch and then duplicate that patch in two trees, one that goes
> through arm-soc and one that goes through clk.
>
Thanks for your suggestion, I will talk about this with Hisilicon SoC
maintainer XuWei.

I have updated this patch based on your suggestion, could you help
review patch "[PATCH v6 5/6] clk: hi6220: Clock driver support for 
Hisilicon hi6220 SoC"?

Thanks,

Bintian

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

[PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Bintian Wang]

Add initial dtsi file to support Hisilicon Hi6220 SoC with
support of Octal core CPUs in two clusters and each cluster
has quard Cortex-A53.

Also add dts file to support HiKey development board which
based on Hi6220 SoC.

Signed-off-by: Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Haojian Zhuang <haojian.zhuang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Yiping Xu <xuyiping-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
---
 arch/arm64/boot/dts/Makefile                   |   1 +
 arch/arm64/boot/dts/hisilicon/Makefile         |   5 +
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts |  31 +++++
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      | 172 +++++++++++++++++++++++++
 4 files changed, 209 insertions(+)
 create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile
 create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
 create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi

diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index ad26a75..38913be 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -4,6 +4,7 @@ dts-dirs += arm
 dts-dirs += cavium
 dts-dirs += exynos
 dts-dirs += freescale
+dts-dirs += hisilicon
 dts-dirs += mediatek
 dts-dirs += qcom
 dts-dirs += sprd
diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile
new file mode 100644
index 0000000..fa81a6e
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/Makefile
@@ -0,0 +1,5 @@
+dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb
+
+always		:= $(dtb-y)
+subdir-y	:= $(dts-dirs)
+clean-files	:= *.dtb
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
new file mode 100644
index 0000000..e36a539
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -0,0 +1,31 @@
+/*
+ * dts file for Hisilicon HiKey Development Board
+ *
+ * Copyright (C) 2015, Hisilicon Ltd.
+ *
+ */
+
+/dts-v1/;
+
+/*Reserved 1MB memory for MCU*/
+/memreserve/ 0x05e00000 0x00100000;
+
+#include "hi6220.dtsi"
+
+/ {
+	model = "HiKey Development Board";
+	compatible = "hisilicon,hi6220-hikey", "hisilicon,hi6220";
+
+	aliases {
+		serial0 = &uart0;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	memory@0 {
+		device_type = "memory";
+		reg = <0x0 0x0 0x0 0x40000000>;
+	};
+};
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
new file mode 100644
index 0000000..e774865
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -0,0 +1,172 @@
+/*
+ * dts file for Hisilicon Hi6220 SoC
+ *
+ * Copyright (C) 2015, Hisilicon Ltd.
+ */
+
+#include <dt-bindings/clock/hi6220-clock.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+	compatible = "hisilicon,hi6220";
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu-map {
+			cluster0 {
+				core0 {
+					cpu = <&cpu0>;
+				};
+				core1 {
+					cpu = <&cpu1>;
+				};
+				core2 {
+					cpu = <&cpu2>;
+				};
+				core3 {
+					cpu = <&cpu3>;
+				};
+			};
+			cluster1 {
+				core0 {
+					cpu = <&cpu4>;
+				};
+				core1 {
+					cpu = <&cpu5>;
+				};
+				core2 {
+					cpu = <&cpu6>;
+				};
+				core3 {
+					cpu = <&cpu7>;
+				};
+			};
+		};
+
+		cpu0: cpu@0 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x0>;
+			enable-method = "psci";
+		};
+
+		cpu1: cpu@1 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x1>;
+			enable-method = "psci";
+		};
+
+		cpu2: cpu@2 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x2>;
+			enable-method = "psci";
+		};
+
+		cpu3: cpu@3 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x3>;
+			enable-method = "psci";
+		};
+
+		cpu4: cpu@100 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x100>;
+			enable-method = "psci";
+		};
+
+		cpu5: cpu@101 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x101>;
+			enable-method = "psci";
+		};
+
+		cpu6: cpu@102 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x102>;
+			enable-method = "psci";
+		};
+
+		cpu7: cpu@103 {
+			compatible = "arm,cortex-a53", "arm,armv8";
+			device_type = "cpu";
+			reg = <0x0 0x103>;
+			enable-method = "psci";
+		};
+	};
+
+	gic: interrupt-controller@f6801000 {
+		compatible = "arm,gic-400";
+		reg = <0x0 0xf6801000 0 0x1000>, /* GICD */
+		      <0x0 0xf6802000 0 0x2000>, /* GICC */
+		      <0x0 0xf6804000 0 0x2000>, /* GICH */
+		      <0x0 0xf6806000 0 0x2000>; /* GICV */
+		#address-cells = <0>;
+		#interrupt-cells = <3>;
+		interrupt-controller;
+		interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupt-parent = <&gic>;
+		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
+	};
+
+	soc {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		ao_ctrl: ao_ctrl {
+			compatible = "hisilicon,hi6220-aoctrl", "syscon";
+			reg = <0x0 0xf7800000 0x0 0x2000>;
+			#clock-cells = <1>;
+		};
+
+		sys_ctrl: sys_ctrl {
+			compatible = "hisilicon,hi6220-sysctrl", "syscon";
+			reg = <0x0 0xf7030000 0x0 0x2000>;
+			#clock-cells = <1>;
+		};
+
+		media_ctrl: media_ctrl {
+			compatible = "hisilicon,hi6220-mediactrl", "syscon";
+			reg = <0x0 0xf4410000 0x0 0x1000>;
+			#clock-cells = <1>;
+		};
+
+		pm_ctrl: pm_ctrl {
+			compatible = "hisilicon,hi6220-pmctrl", "syscon";
+			reg = <0x0 0xf7032000 0x0 0x1000>;
+			#clock-cells = <1>;
+		};
+
+		uart0: uart@f8015000 {	/* console */
+			compatible = "arm,pl011", "arm,primecell";
+			reg = <0x0 0xf8015000 0x0 0x1000>;
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
+			clock-names = "uartclk", "apb_pclk";
+		};
+	};
+};
-- 
1.9.1

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

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland]

Hi,

> +/*Reserved 1MB memory for MCU*/
> +/memreserve/ 0x05e00000 0x00100000;

What exactly is the MCU used for, and what does it use this memory for?

Can this be carved out from the memory node instead? If the OS doesn't
need to access this memory to communicate with the MCU, preventing the
OS from mapping the memory avoids a number of potential issues.

I take it that with UEFI this region is not described to the OS?

[...]

> +	psci {
> +		compatible = "arm,psci-0.2";
> +		method = "smc";
> +	};

Are all the PSCI 0.2 mandatory features implemented?

Can CPU0 be hot unplugged?

[...]

> +		uart0: uart@f8015000 {	/* console */
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x0 0xf8015000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
> +			clock-names = "uartclk", "apb_pclk";
> +		};

In a previous discussion [1] the UART on HI6220 was described as not
fully PL011 compliant, with a number of differences (e.g. the FIFO
length).

Given that, I feel somewhat uncomfortable with the current compatible
string list. What exactly are those differences? We may need a more
specific compatible string (even if in addition to those existing ones),
or perhaps other properties.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Bintian]

Hello Mark,

On 2015/5/6 1:13, Mark Rutland wrote:
> Hi,
>
>> +/*Reserved 1MB memory for MCU*/
>> +/memreserve/ 0x05e00000 0x00100000;
>
> What exactly is the MCU used for, and what does it use this memory for?
>
> Can this be carved out from the memory node instead? If the OS doesn't
> need to access this memory to communicate with the MCU, preventing the
> OS from mapping the memory avoids a number of potential issues.
>
> I take it that with UEFI this region is not described to the OS?

MCU is used for system low power control, the reserved memory is hard
coded by hardware and used by MCU, OS will access this memory to
communicate with the MCU to change the CPU frequency.

>
> [...]
>
>> +	psci {
>> +		compatible = "arm,psci-0.2";
>> +		method = "smc";
>> +	};
>
> Are all the PSCI 0.2 mandatory features implemented?
The system off/suspend is under development, and system off will
be released in next months, and system suspend may be released in the
following two months.

Leo does the development of PSCI, and he can give more detailed plan.

So can I keep "arm,psci-0.2" here?

> Can CPU0 be hot unplugged?
Yes, CPU0~CPU7 all can be hot unplugged.


> [...]
>
>> +		uart0: uart@f8015000 {	/* console */
>> +			compatible = "arm,pl011", "arm,primecell";
>> +			reg = <0x0 0xf8015000 0x0 0x1000>;
>> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
>> +			clock-names = "uartclk", "apb_pclk";
>> +		};
>
> In a previous discussion [1] the UART on HI6220 was described as not
> fully PL011 compliant, with a number of differences (e.g. the FIFO
> length).
>
> Given that, I feel somewhat uncomfortable with the current compatible
> string list. What exactly are those differences? We may need a more
> specific compatible string (even if in addition to those existing ones),
> or perhaps other properties.
The small system can be booted and the console also works well without
changing any code of driver amba-pl011.c, so I think the compatible
string is OK for this patch set.

Hisilicon do some performance enhancements based on PL011, but the
current driver "amba-pl011.c" also works on hi6220 without those
enhancements driver code.

Thanks,

Bintian

>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html
>
> .
>

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Leo Yan]

On Wed, May 06, 2015 at 11:16:12AM +0800, Bintian Wang wrote:
> On 2015/5/6 1:13, Mark Rutland wrote:
> >[...]
> >
> >>+	psci {
> >>+		compatible = "arm,psci-0.2";
> >>+		method = "smc";
> >>+	};
> >
> >Are all the PSCI 0.2 mandatory features implemented?
>
> The system off/suspend is under development, and system off will
> be released in next months, and system suspend may be released in the
> following two months.

Yes; now for mandatory features, except SYSTEM_OFF others have supported.
for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right?

> So can I keep "arm,psci-0.2" here?
> 
> >Can CPU0 be hot unplugged?
> Yes, CPU0~CPU7 all can be hot unplugged.

Just clarify: now use s/w method to emulate CPU and cluster's low
power state; all cores can be hotplugged [1].

[1] https://github.com/96boards/arm-trusted-firmware;

Thanks,
Leo Yan

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland]

On Wed, May 06, 2015 at 04:51:20AM +0100, Leo Yan wrote:
> On Wed, May 06, 2015 at 11:16:12AM +0800, Bintian Wang wrote:
> > On 2015/5/6 1:13, Mark Rutland wrote:
> > >[...]
> > >
> > >>+	psci {
> > >>+		compatible = "arm,psci-0.2";
> > >>+		method = "smc";
> > >>+	};
> > >
> > >Are all the PSCI 0.2 mandatory features implemented?
> >
> > The system off/suspend is under development, and system off will
> > be released in next months, and system suspend may be released in the
> > following two months.
> 
> Yes; now for mandatory features, except SYSTEM_OFF others have supported.

Ok. Do you mean in the next month, or in a number of months?

> for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right?

SYSTEM_SUSPEND was introduced in PSCI 1.0, and is optional.

So long as PSCI_VERSION reports 0.2 or PSCI_FEATURES reports
NOT_SUPPORTED for SYSTEM_SUSPEND, it's ok to not implement.

I guess PSCI_VERSION reports PSCI 0.2?

> > So can I keep "arm,psci-0.2" here?
> > 
> > >Can CPU0 be hot unplugged?
> > Yes, CPU0~CPU7 all can be hot unplugged.
> 
> Just clarify: now use s/w method to emulate CPU and cluster's low
> power state; all cores can be hotplugged [1].

So long as CPUs brought online are reset appropriately, and "offline"
CPUs can't bring down the system somehow, that should be fine. For now
the important part is the interface.

Thanks,
Mark.

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Leo Yan]

hi Mark,

Thanks for detailed reviewing, pls see below inline comments.

On Wed, May 06, 2015 at 10:20:56AM +0100, Mark Rutland wrote:
> On Wed, May 06, 2015 at 04:51:20AM +0100, Leo Yan wrote:
> > On Wed, May 06, 2015 at 11:16:12AM +0800, Bintian Wang wrote:
> > > On 2015/5/6 1:13, Mark Rutland wrote:
> > > >[...]
> > > >
> > > >>+	psci {
> > > >>+		compatible = "arm,psci-0.2";
> > > >>+		method = "smc";
> > > >>+	};
> > > >
> > > >Are all the PSCI 0.2 mandatory features implemented?
> > >
> > > The system off/suspend is under development, and system off will
> > > be released in next months, and system suspend may be released in the
> > > following two months.
> > 
> > Yes; now for mandatory features, except SYSTEM_OFF others have supported.
> 
> Ok. Do you mean in the next month, or in a number of months?

We will finish it in May; the mainly work is to hook this API w/t PMIC
to power down system.

> > for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right?
> 
> SYSTEM_SUSPEND was introduced in PSCI 1.0, and is optional.
> 
> So long as PSCI_VERSION reports 0.2 or PSCI_FEATURES reports
> NOT_SUPPORTED for SYSTEM_SUSPEND, it's ok to not implement.
> 
> I guess PSCI_VERSION reports PSCI 0.2?

This is depend on ARM trusted firmware's implementation, now we are
using ATFv1.1, so it will declare to support PSCI 1.0; pls see below
boot log:

psci: probing for conduit method from DT.
psci: PSCIv1.0 detected in firmware.
psci: Using standard PSCI v0.2 function IDs

> > > So can I keep "arm,psci-0.2" here?
> > > 
> > > >Can CPU0 be hot unplugged?
> > > Yes, CPU0~CPU7 all can be hot unplugged.
> > 
> > Just clarify: now use s/w method to emulate CPU and cluster's low
> > power state; all cores can be hotplugged [1].
> 
> So long as CPUs brought online are reset appropriately, and "offline"
> CPUs can't bring down the system somehow, that should be fine. For now
> the important part is the interface.

Thanks for confirmation.

Thanks,
Leo Yan

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Bintian]

Hello Mark,

On 2015/5/6 11:16, Bintian wrote:
> Hello Mark,
>
> On 2015/5/6 1:13, Mark Rutland wrote:
[...]
>>> +        uart0: uart@f8015000 {    /* console */
>>> +            compatible = "arm,pl011", "arm,primecell";
>>> +            reg = <0x0 0xf8015000 0x0 0x1000>;
>>> +            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>> +            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
>>> HI6220_UART0_PCLK>;
>>> +            clock-names = "uartclk", "apb_pclk";
>>> +        };
>>
>> In a previous discussion [1] the UART on HI6220 was described as not
>> fully PL011 compliant, with a number of differences (e.g. the FIFO
>> length).
>>
>> Given that, I feel somewhat uncomfortable with the current compatible
>> string list. What exactly are those differences? We may need a more
>> specific compatible string (even if in addition to those existing ones),
>> or perhaps other properties.
> The small system can be booted and the console also works well without
> changing any code of driver amba-pl011.c, so I think the compatible
> string is OK for this patch set.
>
> Hisilicon do some performance enhancements based on PL011, but the
> current driver "amba-pl011.c" also works on hi6220 without those
> enhancements driver code.
Checked with Hisilicon chip designer, the UART0 is used for DEBUG
console and compliant with PL011 fully.

Thanks,

Bintian
>
>>
>> Thanks,
>> Mark.
>>
>> [1]
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html
>>
>>
>> .
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> .
>

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland]

On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote:
> Hello Mark,
> 
> On 2015/5/6 11:16, Bintian wrote:
> > Hello Mark,
> >
> > On 2015/5/6 1:13, Mark Rutland wrote:
> [...]
> >>> +        uart0: uart@f8015000 {    /* console */
> >>> +            compatible = "arm,pl011", "arm,primecell";
> >>> +            reg = <0x0 0xf8015000 0x0 0x1000>;
> >>> +            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> >>> +            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
> >>> HI6220_UART0_PCLK>;
> >>> +            clock-names = "uartclk", "apb_pclk";
> >>> +        };
> >>
> >> In a previous discussion [1] the UART on HI6220 was described as not
> >> fully PL011 compliant, with a number of differences (e.g. the FIFO
> >> length).
> >>
> >> Given that, I feel somewhat uncomfortable with the current compatible
> >> string list. What exactly are those differences? We may need a more
> >> specific compatible string (even if in addition to those existing ones),
> >> or perhaps other properties.
> > The small system can be booted and the console also works well without
> > changing any code of driver amba-pl011.c, so I think the compatible
> > string is OK for this patch set.
> >
> > Hisilicon do some performance enhancements based on PL011, but the
> > current driver "amba-pl011.c" also works on hi6220 without those
> > enhancements driver code.
> Checked with Hisilicon chip designer, the UART0 is used for DEBUG
> console and compliant with PL011 fully.

I think that given that we know the UART is not quite a PL011 we should
add an additional compatible string just in case some difference crops
up later that is problematic.

So we'd have something like:

	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

Thanks,
Mark.

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Bintian]

Hello Mark,

On 2015/5/6 17:30, Mark Rutland wrote:
> On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote:
>> Hello Mark,
>>
>> On 2015/5/6 11:16, Bintian wrote:
>>> Hello Mark,
>>>
>>> On 2015/5/6 1:13, Mark Rutland wrote:
>> [...]
>>>>> +        uart0: uart@f8015000 {    /* console */
>>>>> +            compatible = "arm,pl011", "arm,primecell";
>>>>> +            reg = <0x0 0xf8015000 0x0 0x1000>;
>>>>> +            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
>>>>> HI6220_UART0_PCLK>;
>>>>> +            clock-names = "uartclk", "apb_pclk";
>>>>> +        };
>>>>
>>>> In a previous discussion [1] the UART on HI6220 was described as not
>>>> fully PL011 compliant, with a number of differences (e.g. the FIFO
>>>> length).
>>>>
>>>> Given that, I feel somewhat uncomfortable with the current compatible
>>>> string list. What exactly are those differences? We may need a more
>>>> specific compatible string (even if in addition to those existing ones),
>>>> or perhaps other properties.
>>> The small system can be booted and the console also works well without
>>> changing any code of driver amba-pl011.c, so I think the compatible
>>> string is OK for this patch set.
>>>
>>> Hisilicon do some performance enhancements based on PL011, but the
>>> current driver "amba-pl011.c" also works on hi6220 without those
>>> enhancements driver code.
>> Checked with Hisilicon chip designer, the UART0 is used for DEBUG
>> console and compliant with PL011 fully.
>
> I think that given that we know the UART is not quite a PL011 we should
> add an additional compatible string just in case some difference crops
> up later that is problematic.
>
> So we'd have something like:
>
> 	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
>
> That way we can add any optimisations or workarounds later as required.
I understand and thanks for your suggestion.

Can I do not do this work in this patch set? Because I got the
information UART0 is PL011 compatible. Hisilicon uart engineer can do
this work in the future, maybe for UART1/UART2.

Thank you Mark,

BR,

Bintian

>
> Thanks,
> Mark.
>
> .
>

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland]

Hi,

> > I think that given that we know the UART is not quite a PL011 we should
> > add an additional compatible string just in case some difference crops
> > up later that is problematic.
> >
> > So we'd have something like:
> >
> > 	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
> >
> > That way we can add any optimisations or workarounds later as required.
> I understand and thanks for your suggestion.
> 
> Can I do not do this work in this patch set? Because I got the
> information UART0 is PL011 compatible. Hisilicon uart engineer can do
> this work in the future, maybe for UART1/UART2.

I am not asking you to do any driver work for this -- the current driver
should ignore the "hisilicon,hi6220-uart" string and recognise
"arm,pl011".

I am only asking you to add the additional string to the DTS, and to
update the binding document to list the new string. That way if and when
we need the kernel to distinguish between a regular PL011 and the
hi6220-specific variant, the DTB does not need to be updated in order to
do so.

Is UART 0 different from UART1 and UART2?

Thanks,
Mark.

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Brent Wang]

Hi Mark,

2015-05-06 18:55 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
> Hi,
>
>> > I think that given that we know the UART is not quite a PL011 we should
>> > add an additional compatible string just in case some difference crops
>> > up later that is problematic.
>> >
>> > So we'd have something like:
>> >
>> > 	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
>> >
>> > That way we can add any optimisations or workarounds later as required.
>> I understand and thanks for your suggestion.
>>
>> Can I do not do this work in this patch set? Because I got the
>> information UART0 is PL011 compatible. Hisilicon uart engineer can do
>> this work in the future, maybe for UART1/UART2.
>
> I am not asking you to do any driver work for this -- the current driver
> should ignore the "hisilicon,hi6220-uart" string and recognise
> "arm,pl011".
>
> I am only asking you to add the additional string to the DTS, and to
> update the binding document to list the new string. That way if and when
> we need the kernel to distinguish between a regular PL011 and the
> hi6220-specific variant, the DTB does not need to be updated in order to
> do so.
How about add the following binding rule to my 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"
- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

uart0: uart@f8015000 {
        compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
        reg = <0x0 0xf8015000 0x0 0x1000>;
        interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
        clock-names = "uartclk", "apb_pclk";
};
---------------------------
> Is UART 0 different from UART1 and UART2?
Yes,  but my patch just includes UART0, we do some changements for UART1/2
to improve performance.

Thanks,

Bintian
>
> Thanks,
> Mark.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
Best Regards,

Bintian
===========================
Don't be nervous, just be happy!

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland]

Hi,

> How about add the following binding rule to my 2/5 patch:
> ---------------------------
> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART
> 
> Required properties:
> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"

"arm,primecell" should come last.

> - reg: exactly one register range with length 0x1000
> - interrupts: exactly one interrupt specifier
> 
> See also bindings/serial/pl011.txt
> 
> Example:
> 
> uart0: uart@f8015000 {
>         compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
>         reg = <0x0 0xf8015000 0x0 0x1000>;
>         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>         clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
>         clock-names = "uartclk", "apb_pclk";
> };

How about for the moment we just modify the existing pl011 binding
document to allow for the hi6220-specific string in addition to its
existing strings? We can always split it out later if the hi6220 UART
binding needs to be significantly divergent.

> > Is UART 0 different from UART1 and UART2?
> Yes,  but my patch just includes UART0, we do some changements for UART1/2
> to improve performance.

Sure, but we need to make sure we can choose a sane compatible string
for them, that won't clash with whatever we choose for UART0.

Are they the same IP as UART0, or fundamentally different?

Are they also PL011 derived?

Thanks,
Mark.

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Brent Wang]

Hi Mark,

2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
> Hi,
>
>> How about add the following binding rule to my 2/5 patch:
>> ---------------------------
>> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART
>>
>> Required properties:
>> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell",
>> "arm,pl011"
>
> "arm,primecell" should come last.
>
>> - reg: exactly one register range with length 0x1000
>> - interrupts: exactly one interrupt specifier
>>
>> See also bindings/serial/pl011.txt
>>
>> Example:
>>
>> uart0: uart@f8015000 {
>>         compatible = "hisilicon,hi6220-uart", "arm,pl011",
>> "arm,primecell";
>>         reg = <0x0 0xf8015000 0x0 0x1000>;
>>         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>         clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
>> HI6220_UART0_PCLK>;
>>         clock-names = "uartclk", "apb_pclk";
>> };
>
> How about for the moment we just modify the existing pl011 binding
> document to allow for the hi6220-specific string in addition to its
> existing strings? We can always split it out later if the hi6220 UART
> binding needs to be significantly divergent.
How about adding the following rule to pl011.txt?
----------
diff --git a/Documentation/devicetree/bindings/serial/pl011.txt
b/Documentation/devicetree/bindings/serial/pl011.txt
index ba3ecb8..1221ccb 100644
--- a/Documentation/devicetree/bindings/serial/pl011.txt
+++ b/Documentation/devicetree/bindings/serial/pl011.txt
@@ -35,6 +35,10 @@ Optional properties:
 - poll-timeout-ms:
           Poll timeout when auto-poll is set, default
           3000ms.
+- compatible: "hisilicon,hi6220-uart":
+          Hisilicon does some enhancements (e.g. larger FIFO length)
+           based on PL011, so when using these UART hosts, this compatible
+           string should be added.

 See also bindings/arm/primecell.txt
------------
>
>> > Is UART 0 different from UART1 and UART2?
>> Yes,  but my patch just includes UART0, we do some changements for
>> UART1/2
>> to improve performance.
>
> Sure, but we need to make sure we can choose a sane compatible string
> for them, that won't clash with whatever we choose for UART0.
OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 node?

>
> Are they the same IP as UART0, or fundamentally different?
The same IP.
>
> Are they also PL011 derived?
Yes, just do some enhancements to improve performance.

Thanks,

Bintian

>
> Thanks,
> Mark.
>


-- 
Best Regards,

Bintian
===========================
Don't be nervous, just be happy!

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland]

On Wed, May 06, 2015 at 05:03:29PM +0100, Brent Wang wrote:
> Hi Mark,
> 
> 2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
> > Hi,
> >
> >> How about add the following binding rule to my 2/5 patch:
> >> ---------------------------
> >> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART
> >>
> >> Required properties:
> >> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell",
> >> "arm,pl011"
> >
> > "arm,primecell" should come last.
> >
> >> - reg: exactly one register range with length 0x1000
> >> - interrupts: exactly one interrupt specifier
> >>
> >> See also bindings/serial/pl011.txt
> >>
> >> Example:
> >>
> >> uart0: uart@f8015000 {
> >>         compatible = "hisilicon,hi6220-uart", "arm,pl011",
> >> "arm,primecell";
> >>         reg = <0x0 0xf8015000 0x0 0x1000>;
> >>         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> >>         clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
> >> HI6220_UART0_PCLK>;
> >>         clock-names = "uartclk", "apb_pclk";
> >> };
> >
> > How about for the moment we just modify the existing pl011 binding
> > document to allow for the hi6220-specific string in addition to its
> > existing strings? We can always split it out later if the hi6220 UART
> > binding needs to be significantly divergent.
> How about adding the following rule to pl011.txt?
> ----------
> diff --git a/Documentation/devicetree/bindings/serial/pl011.txt
> b/Documentation/devicetree/bindings/serial/pl011.txt
> index ba3ecb8..1221ccb 100644
> --- a/Documentation/devicetree/bindings/serial/pl011.txt
> +++ b/Documentation/devicetree/bindings/serial/pl011.txt
> @@ -35,6 +35,10 @@ Optional properties:
>  - poll-timeout-ms:
>            Poll timeout when auto-poll is set, default
>            3000ms.
> +- compatible: "hisilicon,hi6220-uart":
> +          Hisilicon does some enhancements (e.g. larger FIFO length)
> +           based on PL011, so when using these UART hosts, this compatible
> +           string should be added.

Up at the top of the document there's already a description of the
compatible property. Just change it into a list something like:

- compatible: should contain one of the following sequences:
  * "arm,pl011", "arm,primecell"
  * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell"

>  See also bindings/arm/primecell.txt
> ------------
> >
> >> > Is UART 0 different from UART1 and UART2?
> >> Yes,  but my patch just includes UART0, we do some changements for
> >> UART1/2
> >> to improve performance.
> >
> > Sure, but we need to make sure we can choose a sane compatible string
> > for them, that won't clash with whatever we choose for UART0.
> OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 node?

This depends on a number of factors. Questions below.

> > Are they the same IP as UART0, or fundamentally different?
> The same IP.
> >
> > Are they also PL011 derived?
> Yes, just do some enhancements to improve performance.

What exactly is different between UART0 and UART{1,2}, given they are
the same IP?

Can UART{1,2} be used as if they are standard PL011 instances?

Is the difference internal, or do they just have different
clocks/regulators/etc?

Do they have feature control pins tied off differently?

Are they simply programmed differently at reset by firmware?

Thanks,
Mark.

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Brent Wang]

Hi Mark,

2015-05-07 0:23 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
> On Wed, May 06, 2015 at 05:03:29PM +0100, Brent Wang wrote:
>> Hi Mark,
>>
>> 2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
>> > Hi,
>> >
>> >> How about add the following binding rule to my 2/5 patch:
>> >> ---------------------------
>> >> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART
>> >>
>> >> Required properties:
>> >> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell",
>> >> "arm,pl011"
>> >
>> > "arm,primecell" should come last.
>> >
>> >> - reg: exactly one register range with length 0x1000
>> >> - interrupts: exactly one interrupt specifier
>> >>
>> >> See also bindings/serial/pl011.txt
>> >>
>> >> Example:
>> >>
>> >> uart0: uart@f8015000 {
>> >>         compatible = "hisilicon,hi6220-uart", "arm,pl011",
>> >> "arm,primecell";
>> >>         reg = <0x0 0xf8015000 0x0 0x1000>;
>> >>         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>> >>         clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
>> >> HI6220_UART0_PCLK>;
>> >>         clock-names = "uartclk", "apb_pclk";
>> >> };
>> >
>> > How about for the moment we just modify the existing pl011 binding
>> > document to allow for the hi6220-specific string in addition to its
>> > existing strings? We can always split it out later if the hi6220 UART
>> > binding needs to be significantly divergent.
>> How about adding the following rule to pl011.txt?
>> ----------
>> diff --git a/Documentation/devicetree/bindings/serial/pl011.txt
>> b/Documentation/devicetree/bindings/serial/pl011.txt
>> index ba3ecb8..1221ccb 100644
>> --- a/Documentation/devicetree/bindings/serial/pl011.txt
>> +++ b/Documentation/devicetree/bindings/serial/pl011.txt
>> @@ -35,6 +35,10 @@ Optional properties:
>>  - poll-timeout-ms:
>>            Poll timeout when auto-poll is set, default
>>            3000ms.
>> +- compatible: "hisilicon,hi6220-uart":
>> +          Hisilicon does some enhancements (e.g. larger FIFO length)
>> +           based on PL011, so when using these UART hosts, this
>> compatible
>> +           string should be added.
>
> Up at the top of the document there's already a description of the
> compatible property. Just change it into a list something like:
>
> - compatible: should contain one of the following sequences:
>   * "arm,pl011", "arm,primecell"
>   * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell"
OK, I will add in next version.

>
>>  See also bindings/arm/primecell.txt
>> ------------
>> >
>> >> > Is UART 0 different from UART1 and UART2?
>> >> Yes,  but my patch just includes UART0, we do some changements for
>> >> UART1/2
>> >> to improve performance.
>> >
>> > Sure, but we need to make sure we can choose a sane compatible string
>> > for them, that won't clash with whatever we choose for UART0.
>> OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0
>> node?
>
> This depends on a number of factors. Questions below.
>
>> > Are they the same IP as UART0, or fundamentally different?
>> The same IP.
>> >
>> > Are they also PL011 derived?
>> Yes, just do some enhancements to improve performance.
>
> What exactly is different between UART0 and UART{1,2}, given they are
> the same IP?
I know they are the same IP, but UART{1,2} have larger FIFO length.

>
> Can UART{1,2} be used as if they are standard PL011 instances?
Yes

> Is the difference internal, or do they just have different
> clocks/regulators/etc?
The different FIFO length means internal?  Sure, they have different
clocks/regualtors.
>
> Do they have feature control pins tied off differently?
UART{1,2} have reset function, we must disable the reset before using
them, is it the feature
control pins you mentioned ?
>
> Are they simply programmed differently at reset by firmware?
I think so.

For hi6220 uart{1,2} , I think add one "struct vendor_data
vendor_hisilicon" to "amba-pl011.c" may be a good solution in the
future.

I am not the uart driver developer and the Hisilicon engineer who
develop it may have a deep
discussion with you when upstreaming UART{1,2}.

I suggest not add "hisilicon,hi6220-uart" to UART0;

Must go to bed, it's very late here :(

Thanks,

Bintian
>
> Thanks,
> Mark.
>


-- 
Best Regards,

Bintian
===========================
Don't be nervous, just be happy!

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Bintian]

Hi Mark,

On 2015/5/7 1:15, Brent Wang wrote:
> Hi Mark,
>
> 2015-05-07 0:23 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
>> On Wed, May 06, 2015 at 05:03:29PM +0100, Brent Wang wrote:
>>> Hi Mark,
>>>
>>> 2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
>>>> Hi,
>>>>
>>>>> How about add the following binding rule to my 2/5 patch:
>>>>> ---------------------------
>>>>> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART
>>>>>
>>>>> Required properties:
>>>>> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell",
>>>>> "arm,pl011"
>>>>
>>>> "arm,primecell" should come last.
>>>>
>>>>> - reg: exactly one register range with length 0x1000
>>>>> - interrupts: exactly one interrupt specifier
>>>>>
>>>>> See also bindings/serial/pl011.txt
>>>>>
>>>>> Example:
>>>>>
>>>>> uart0: uart@f8015000 {
>>>>>          compatible = "hisilicon,hi6220-uart", "arm,pl011",
>>>>> "arm,primecell";
>>>>>          reg = <0x0 0xf8015000 0x0 0x1000>;
>>>>>          interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>>>>          clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
>>>>> HI6220_UART0_PCLK>;
>>>>>          clock-names = "uartclk", "apb_pclk";
>>>>> };
>>>>
>>>> How about for the moment we just modify the existing pl011 binding
>>>> document to allow for the hi6220-specific string in addition to its
>>>> existing strings? We can always split it out later if the hi6220 UART
>>>> binding needs to be significantly divergent.
>>> How about adding the following rule to pl011.txt?
>>> ----------
>>> diff --git a/Documentation/devicetree/bindings/serial/pl011.txt
>>> b/Documentation/devicetree/bindings/serial/pl011.txt
>>> index ba3ecb8..1221ccb 100644
>>> --- a/Documentation/devicetree/bindings/serial/pl011.txt
>>> +++ b/Documentation/devicetree/bindings/serial/pl011.txt
>>> @@ -35,6 +35,10 @@ Optional properties:
>>>   - poll-timeout-ms:
>>>             Poll timeout when auto-poll is set, default
>>>             3000ms.
>>> +- compatible: "hisilicon,hi6220-uart":
>>> +          Hisilicon does some enhancements (e.g. larger FIFO length)
>>> +           based on PL011, so when using these UART hosts, this
>>> compatible
>>> +           string should be added.
>>
>> Up at the top of the document there's already a description of the
>> compatible property. Just change it into a list something like:
>>
>> - compatible: should contain one of the following sequences:
>>    * "arm,pl011", "arm,primecell"
>>    * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell"
> OK, I will add in next version.
>
>>
>>>   See also bindings/arm/primecell.txt
>>> ------------
>>>>
>>>>>> Is UART 0 different from UART1 and UART2?
>>>>> Yes,  but my patch just includes UART0, we do some changements for
>>>>> UART1/2
>>>>> to improve performance.
>>>>
>>>> Sure, but we need to make sure we can choose a sane compatible string
>>>> for them, that won't clash with whatever we choose for UART0.
>>> OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0
>>> node?
>>
>> This depends on a number of factors. Questions below.
>>
>>>> Are they the same IP as UART0, or fundamentally different?
>>> The same IP.
>>>>
>>>> Are they also PL011 derived?
>>> Yes, just do some enhancements to improve performance.
>>
>> What exactly is different between UART0 and UART{1,2}, given they are
>> the same IP?
> I know they are the same IP, but UART{1,2} have larger FIFO length.
>
>>
>> Can UART{1,2} be used as if they are standard PL011 instances?
> Yes
>
>> Is the difference internal, or do they just have different
>> clocks/regulators/etc?
> The different FIFO length means internal?  Sure, they have different
> clocks/regualtors.
>>
>> Do they have feature control pins tied off differently?
> UART{1,2} have reset function, we must disable the reset before using
> them, is it the feature
> control pins you mentioned ?
>>
>> Are they simply programmed differently at reset by firmware?
> I think so.
>
> For hi6220 uart{1,2} , I think add one "struct vendor_data
> vendor_hisilicon" to "amba-pl011.c" may be a good solution in the
> future.
>
> I am not the uart driver developer and the Hisilicon engineer who
> develop it may have a deep
> discussion with you when upstreaming UART{1,2}.
>
> I suggest not add "hisilicon,hi6220-uart" to UART0;
How about not adding "hisilicon,hi6220-uart" to UART0, and just add this
compatible string for future use?  you know, the UART0 is PL011
compatible and I checked this with chip designer.

If there is no problem, I will send another version of this patch set
and add this compatible string to pl011.txt as you suggested.

Thanks,

Bintian

>
> Must go to bed, it's very late here :(
>
> Thanks,
>
> Bintian
>>
>> Thanks,
>> Mark.
>>
>
>

Re: Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Bintian Wang]

Hi Mark,

> Hi,
> 
> > > I think that given that we know the UART is not quite a PL011 we should
> > > add an additional compatible string just in case some difference crops
> > > up later that is problematic.
> > >
> > > So we'd have something like:
> > >
> > > 	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
> > >
> > > That way we can add any optimisations or workarounds later as required.
> > I understand and thanks for your suggestion.
> > 
> > Can I do not do this work in this patch set? Because I got the
> > information UART0 is PL011 compatible. Hisilicon uart engineer can do
> > this work in the future, maybe for UART1/UART2.
> 
> I am not asking you to do any driver work for this -- the current driver
> should ignore the "hisilicon,hi6220-uart" string and recognise
> "arm,pl011".
> 
> I am only asking you to add the additional string to the DTS, and to
> update the binding document to list the new string. That way if and when
> we need the kernel to distinguish between a regular PL011 and the
> hi6220-specific variant, the DTB does not need to be updated in order to
> do so.
How about add the following binding rule to the 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"
- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

uart0: uart@f8015000 { 
        compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
        reg = <0x0 0xf8015000 0x0 0x1000>;
        interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
        clock-names = "uartclk", "apb_pclk";
};
---------------------------
 
> Is UART 0 different from UART1 and UART2?
Yes,  but my patch just includes UART0, we do some changements for UART1/2
to improve performance.  

Thanks,

Bintian

> 
> Thanks,
> Mark.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Bintian]

Please ignore this email.

On 2015/5/13 15:12, Bintian Wang wrote:
> Hi Mark,
>
>> Hi,
>>
>>>> I think that given that we know the UART is not quite a PL011 we should
>>>> add an additional compatible string just in case some difference crops
>>>> up later that is problematic.
>>>>
>>>> So we'd have something like:
>>>>
>>>> 	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
>>>>
>>>> That way we can add any optimisations or workarounds later as required.
>>> I understand and thanks for your suggestion.
>>>
>>> Can I do not do this work in this patch set? Because I got the
>>> information UART0 is PL011 compatible. Hisilicon uart engineer can do
>>> this work in the future, maybe for UART1/UART2.
>>
>> I am not asking you to do any driver work for this -- the current driver
>> should ignore the "hisilicon,hi6220-uart" string and recognise
>> "arm,pl011".
>>
>> I am only asking you to add the additional string to the DTS, and to
>> update the binding document to list the new string. That way if and when
>> we need the kernel to distinguish between a regular PL011 and the
>> hi6220-specific variant, the DTB does not need to be updated in order to
>> do so.
> How about add the following binding rule to the 2/5 patch:
> ---------------------------
> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART
>
> Required properties:
> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"
> - reg: exactly one register range with length 0x1000
> - interrupts: exactly one interrupt specifier
>
> See also bindings/serial/pl011.txt
>
> Example:
>
> uart0: uart@f8015000 {
>          compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
>          reg = <0x0 0xf8015000 0x0 0x1000>;
>          interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>          clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
>          clock-names = "uartclk", "apb_pclk";
> };
> ---------------------------
>
>> Is UART 0 different from UART1 and UART2?
> Yes,  but my patch just includes UART0, we do some changements for UART1/2
> to improve performance.
>
> Thanks,
>
> Bintian
>
>>
>> Thanks,
>> Mark.
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
>
>
> .
>

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Haojian Zhuang]

On 6 May 2015 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote:
>> Hello Mark,
>>
>> On 2015/5/6 11:16, Bintian wrote:
>> > Hello Mark,
>> >
>> > On 2015/5/6 1:13, Mark Rutland wrote:
>> [...]
>> >>> +        uart0: uart@f8015000 {    /* console */
>> >>> +            compatible = "arm,pl011", "arm,primecell";
>> >>> +            reg = <0x0 0xf8015000 0x0 0x1000>;
>> >>> +            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>> >>> +            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
>> >>> HI6220_UART0_PCLK>;
>> >>> +            clock-names = "uartclk", "apb_pclk";
>> >>> +        };
>> >>
>> >> In a previous discussion [1] the UART on HI6220 was described as not
>> >> fully PL011 compliant, with a number of differences (e.g. the FIFO
>> >> length).
>> >>
>> >> Given that, I feel somewhat uncomfortable with the current compatible
>> >> string list. What exactly are those differences? We may need a more
>> >> specific compatible string (even if in addition to those existing ones),
>> >> or perhaps other properties.
>> > The small system can be booted and the console also works well without
>> > changing any code of driver amba-pl011.c, so I think the compatible
>> > string is OK for this patch set.
>> >
>> > Hisilicon do some performance enhancements based on PL011, but the
>> > current driver "amba-pl011.c" also works on hi6220 without those
>> > enhancements driver code.
>> Checked with Hisilicon chip designer, the UART0 is used for DEBUG
>> console and compliant with PL011 fully.
>
> I think that given that we know the UART is not quite a PL011 we should
> add an additional compatible string just in case some difference crops
> up later that is problematic.
>
> So we'd have something like:
>
>         compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
>
> That way we can add any optimisations or workarounds later as required.
>

There's no code to handle the feature of hi6220 uart in mainline. It's
meaningless to append "hisilicon,hi6220-uart" at here. I think it's
only required
when those uart code are merged into mainline.

Regards
Haojian

Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland]

On Wed, May 06, 2015 at 11:38:43AM +0100, Haojian Zhuang wrote:
> On 6 May 2015 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote:
> >> Hello Mark,
> >>
> >> On 2015/5/6 11:16, Bintian wrote:
> >> > Hello Mark,
> >> >
> >> > On 2015/5/6 1:13, Mark Rutland wrote:
> >> [...]
> >> >>> +        uart0: uart@f8015000 {    /* console */
> >> >>> +            compatible = "arm,pl011", "arm,primecell";
> >> >>> +            reg = <0x0 0xf8015000 0x0 0x1000>;
> >> >>> +            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> >> >>> +            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
> >> >>> HI6220_UART0_PCLK>;
> >> >>> +            clock-names = "uartclk", "apb_pclk";
> >> >>> +        };
> >> >>
> >> >> In a previous discussion [1] the UART on HI6220 was described as not
> >> >> fully PL011 compliant, with a number of differences (e.g. the FIFO
> >> >> length).
> >> >>
> >> >> Given that, I feel somewhat uncomfortable with the current compatible
> >> >> string list. What exactly are those differences? We may need a more
> >> >> specific compatible string (even if in addition to those existing ones),
> >> >> or perhaps other properties.
> >> > The small system can be booted and the console also works well without
> >> > changing any code of driver amba-pl011.c, so I think the compatible
> >> > string is OK for this patch set.
> >> >
> >> > Hisilicon do some performance enhancements based on PL011, but the
> >> > current driver "amba-pl011.c" also works on hi6220 without those
> >> > enhancements driver code.
> >> Checked with Hisilicon chip designer, the UART0 is used for DEBUG
> >> console and compliant with PL011 fully.
> >
> > I think that given that we know the UART is not quite a PL011 we should
> > add an additional compatible string just in case some difference crops
> > up later that is problematic.
> >
> > So we'd have something like:
> >
> >         compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
> >
> > That way we can add any optimisations or workarounds later as required.
> >
> 
> There's no code to handle the feature of hi6220 uart in mainline.

Sure, I wasn't suggesting that there was.

> It's meaningless to append "hisilicon,hi6220-uart" at here. I think
> it's only required when those uart code are merged into mainline.

I disagree. Given that the UART is not quite a PL011, it is likely to
behave differently under certain circumstances, and may have its own
errata that need to be worked around.

Even if the driver happens to currently work with it, a driver change
could easily break that. By being explicit from the outset that this is
a hi6220-specific PL011 variant, we can target workarounds far more
effectively later, without requiring DTB updates.

Having the additional string costs us nothing today.

Thanks,
Mark.

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Haojian Zhuang]

On 5 May 2015 at 20:06, Bintian Wang <bintian.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
> Hi6220 is one mobile solution of Hisilicon, this patchset contains
> initial support for Hi6220 SoC and HiKey development board, which
> supports octal ARM Cortex A53 cores. Initial support is minimal and
> includes just the arch configuration, clock driver, device tree
> configuration.
>
> PSCI is enabled in device tree and there is no problem to boot all the
> octal cores, and the CPU hotplug is also working now, you can download
> and compile the latest firmware based on the following link to run this
> patch set:
> https://github.com/96boards/documentation/wiki/UEFI
>
> Changes v4:
> * Rebase to kernel 4.1-rc1
> * Delete "arm,cortex-a15-gic" from the gic node in dts
>

Acked-by: Haojian Zhuang <haojian.zhuang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Bintian]

Hello Mark, Will,

Any other suggestions for this patch set?

Thanks,

Bintian

On 2015/5/5 20:06, Bintian Wang wrote:
> Hi6220 is one mobile solution of Hisilicon, this patchset contains
> initial support for Hi6220 SoC and HiKey development board, which
> supports octal ARM Cortex A53 cores. Initial support is minimal and
> includes just the arch configuration, clock driver, device tree
> configuration.
>
> PSCI is enabled in device tree and there is no problem to boot all the
> octal cores, and the CPU hotplug is also working now, you can download
> and compile the latest firmware based on the following link to run this
> patch set:
> https://github.com/96boards/documentation/wiki/UEFI
>
> Changes v4:
> * Rebase to kernel 4.1-rc1
> * Delete "arm,cortex-a15-gic" from the gic node in dts
>
> Changes v3:
> * Verified the CPU hotplug based on the new released firmware
> * Redefined the compatible strings of four system controllers in dts
> * Setting COMMON_CLK_HI6220 to a bool symbol
> * Keep CONFGI_ARCH_HISI sorted alphabetically
>
> Changes v2:
> * Split the DT bindings documents into earlier patches
> * Change SMP enable method from spin-table to PSCI in device tree
> * Remove "clock-frequency" from armv8-timer device node in device tree
> * Add more description about Hisilicon designed system controllers
>    in DT bindings document
> * Enable high speed clock on UART1 mux
> * Other changes based on the discussion in the mailing list:
>    https://lkml.org/lkml/2015/2/5/147
>
> Bintian Wang (5):
>    arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig
>    arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC
>    clk: hi6220: Document devicetree bindings for hi6220 clock
>    clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
>    arm64: dts: Add dts files for Hisilicon Hi6220 SoC
>
>   .../bindings/arm/hisilicon/hisilicon.txt           |  87 ++++++
>   .../devicetree/bindings/clock/hi6220-clock.txt     |  34 +++
>   arch/arm64/Kconfig                                 |   5 +
>   arch/arm64/boot/dts/Makefile                       |   1 +
>   arch/arm64/boot/dts/hisilicon/Makefile             |   5 +
>   arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  31 +++
>   arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 172 ++++++++++++
>   arch/arm64/configs/defconfig                       |   1 +
>   drivers/clk/Kconfig                                |   2 +
>   drivers/clk/Makefile                               |   4 +-
>   drivers/clk/hisilicon/Kconfig                      |   6 +
>   drivers/clk/hisilicon/Makefile                     |   3 +-
>   drivers/clk/hisilicon/clk-hi6220.c                 | 292 +++++++++++++++++++++
>   drivers/clk/hisilicon/clk.c                        |  29 ++
>   drivers/clk/hisilicon/clk.h                        |  17 ++
>   drivers/clk/hisilicon/clkdivider-hi6220.c          | 273 +++++++++++++++++++
>   include/dt-bindings/clock/hi6220-clock.h           | 173 ++++++++++++
>   17 files changed, 1131 insertions(+), 4 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt
>   create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile
>   create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>   create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>   create mode 100644 drivers/clk/hisilicon/Kconfig
>   create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
>   create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c
>   create mode 100644 include/dt-bindings/clock/hi6220-clock.h
>

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

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Will Deacon]

On Wed, May 13, 2015 at 08:33:45AM +0100, Bintian wrote:
> Any other suggestions for this patch set?

You sent a v5, right, so this version (4) has been superseded?
The patches should all go via arm-soc once you've got the relevant acks
in place (I don't think you need anything from me or Catalin -- the Kconfig
changes are trivial).

Will

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Arnd Bergmann]

On Wednesday 13 May 2015 10:16:44 Will Deacon wrote:
> On Wed, May 13, 2015 at 08:33:45AM +0100, Bintian wrote:
> > Any other suggestions for this patch set?
> 
> You sent a v5, right, so this version (4) has been superseded?
> The patches should all go via arm-soc once you've got the relevant acks
> in place (I don't think you need anything from me or Catalin -- the Kconfig
> changes are trivial).
> 
> 

To be more specific here, I expect the patches to be picked up by Wei Xu
and forwarded to arm@kernel.org when he's made sure that everybody
including himself is happy with the outcome.

	Arnd

Re: [PATCH v4 0/5] arm64,hi6220: Enable Hisilicon Hi6220 SoC

[Bintian]

Hello Will,

On 2015/5/13 17:16, Will Deacon wrote:
> On Wed, May 13, 2015 at 08:33:45AM +0100, Bintian wrote:
>> Any other suggestions for this patch set?
>
> You sent a v5, right, so this version (4) has been superseded?
Sorry for my mistake, I just want to get feedback about v5, but send
this email with v4, v5 includes some changes based on some suggestions
from Mark.

Please consider the version (5).

Thanks,

Bintian

> The patches should all go via arm-soc once you've got the relevant acks
> in place (I don't think you need anything from me or Catalin -- the Kconfig
> changes are trivial).
>
> Will
>
> .
>