2015-12-03 03:22:54

by Jiancheng Xue

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

add dts files for hi3519-demb board

Signed-off-by: Jiancheng Xue <[email protected]>
---
arch/arm/boot/dts/Makefile | 2 +
arch/arm/boot/dts/hi3519-demb.dts | 55 ++++++++++++++++
arch/arm/boot/dts/hi3519.dtsi | 129 ++++++++++++++++++++++++++++++++++++++
3 files changed, 186 insertions(+)
create mode 100644 arch/arm/boot/dts/hi3519-demb.dts
create mode 100644 arch/arm/boot/dts/hi3519.dtsi

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 30bbc37..39ad947 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -133,6 +133,8 @@ dtb-$(CONFIG_ARCH_EXYNOS5) += \
exynos5440-sd5v1.dtb \
exynos5440-ssdk5440.dtb \
exynos5800-peach-pi.dtb
+dtb-$(CONFIG_ARCH_HI3519) += \
+ hi3519-demb.dtb
dtb-$(CONFIG_ARCH_HI3xxx) += \
hi3620-hi4511.dtb
dtb-$(CONFIG_ARCH_HIX5HD2) += \
diff --git a/arch/arm/boot/dts/hi3519-demb.dts b/arch/arm/boot/dts/hi3519-demb.dts
new file mode 100644
index 0000000..c7a1720
--- /dev/null
+++ b/arch/arm/boot/dts/hi3519-demb.dts
@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+/dts-v1/;
+#include "hi3519.dtsi"
+
+/ {
+ model = "HiSilicon HI3519 DEMO Board";
+ compatible = "hisilicon,hi3519";
+
+ chosen {
+ bootargs = "mem=64M console=ttyAMA0,115200 early_printk \
+root=/dev/mtdblock2 rootfstype=jffs2 \
+mtdparts=hi_sfc:1M(boot),4M(kernel),11M(rootfs)";
+ };
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a7";
+ reg = <0>;
+ };
+ };
+
+ memory {
+ device_type = "memory";
+ reg = <0x80000000 0x40000000>;
+ };
+};
+
+&uart0 {
+ status = "okay";
+};
+
+&dual_timer0 {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/hi3519.dtsi b/arch/arm/boot/dts/hi3519.dtsi
new file mode 100644
index 0000000..32f08e6
--- /dev/null
+++ b/arch/arm/boot/dts/hi3519.dtsi
@@ -0,0 +1,129 @@
+/*
+ * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "skeleton.dtsi"
+#include <dt-bindings/clock/hi3519-clock.h>
+/ {
+ aliases {
+ serial0 = &uart0;
+ };
+
+ gic: interrupt-controller@10300000 {
+ compatible = "arm,cortex-a7-gic";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
+ };
+
+ soc {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "simple-bus";
+ interrupt-parent = <&gic>;
+ ranges;
+
+ amba {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "arm,amba-bus";
+ ranges;
+
+ uart0: uart@12100000 {
+ compatible = "arm,pl011", "arm,primecell";
+ reg = <0x12100000 0x1000>;
+ interrupts = <0 4 4>;
+ clocks = <&crg HI3519_UART0_CLK>;
+ clock-names = "apb_pclk";
+ status = "disable";
+ };
+
+ uart1: uart@12101000 {
+ compatible = "arm,pl011", "arm,primecell";
+ reg = <0x12101000 0x1000>;
+ interrupts = <0 5 4>;
+ clocks = <&crg HI3519_UART1_CLK>;
+ clock-names = "apb_pclk";
+ status = "disable";
+ };
+
+ uart2: uart@12102000 {
+ compatible = "arm,pl011", "arm,primecell";
+ reg = <0x12102000 0x1000>;
+ interrupts = <0 6 4>;
+ clocks = <&crg HI3519_UART2_CLK>;
+ clock-names = "apb_pclk";
+ status = "disable";
+ };
+
+ uart3: uart@12103000 {
+ compatible = "arm,pl011", "arm,primecell";
+ reg = <0x12103000 0x1000>;
+ interrupts = <0 7 4>;
+ clocks = <&crg HI3519_UART3_CLK>;
+ clock-names = "apb_pclk";
+ status = "disable";
+ };
+
+ uart4: uart@12104000 {
+ compatible = "arm,pl011", "arm,primecell";
+ reg = <0x12104000 0x1000>;
+ interrupts = <0 8 4>;
+ clocks = <&crg HI3519_UART4_CLK>;
+ clock-names = "apb_pclk";
+ status = "disable";
+ };
+
+ dual_timer0: dual_timer@12000000 {
+ compatible = "arm,sp804", "arm,primecell";
+ interrupts = <0 64 4>, <0 65 4>;
+ reg = <0x12000000 0x1000>;
+ clocks = <&crg HI3519_FIXED_3M>;
+ status = "disable";
+ };
+
+ dual_timer1: dual_timer@12001000 {
+ compatible = "arm,sp804", "arm,primecell";
+ interrupts = <0 66 4>, <0 67 4>;
+ reg = <0x12001000 0x1000>;
+ clocks = <&crg HI3519_FIXED_3M>;
+ status = "disable";
+ };
+
+ dual_timer2: dual_timer@12002000 {
+ compatible = "arm,sp804", "arm,primecell";
+ interrupts = <0 68 4>, <0 69 4>;
+ reg = <0x12002000 0x1000>;
+ clocks = <&crg HI3519_FIXED_3M>;
+ status = "disable";
+ };
+ };
+
+ sysctrl: system-controller@12020000 {
+ compatible = "hisilicon,sysctrl";
+ reg = <0x12020000 0x1000>;
+ reboot-offset = <0x4>;
+ };
+
+ crg: crg@12010000 {
+ compatible = "hisilicon,hi3519-crg";
+ #clock-cells = <1>;
+ #reset-cells = <2>;
+ reg = <0x12010000 0x10000>;
+ };
+ };
+};
--
1.9.1


2015-12-03 09:37:12

by Arnd Bergmann

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

On Thursday 03 December 2015 10:44:28 Jiancheng Xue wrote:

> +
> +/dts-v1/;
> +#include "hi3519.dtsi"
> +
> +/ {
> + model = "HiSilicon HI3519 DEMO Board";
> + compatible = "hisilicon,hi3519";
> +
> + chosen {
> + bootargs = "mem=64M console=ttyAMA0,115200 early_printk \
> +root=/dev/mtdblock2 rootfstype=jffs2 \
> +mtdparts=hi_sfc:1M(boot),4M(kernel),11M(rootfs)";
> + };

Most of the arguments should be dropped and replaced with the respective
DT properties in this file:

mem: /memory (you have that already, but the size seems wrong)
console: /chosen/stdout-path
early_printk: just drop this, maybe use "earlycon")
root: this one is fine
rootfstype: should not be needed
mtdparts: use nodes below the MTD device


> +
> +#include "skeleton.dtsi"
> +#include <dt-bindings/clock/hi3519-clock.h>
> +/ {
> + aliases {
> + serial0 = &uart0;
> + };

Move this into the .dts file.

> +
> + uart0: uart@12100000 {

rename to serial@12100000

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

rename to timer@12001000

> + sysctrl: system-controller@12020000 {
> + compatible = "hisilicon,sysctrl";
> + reg = <0x12020000 0x1000>;
> + reboot-offset = <0x4>;
> + };

Is this one identical to the one in hip04?

If not, pick a new unique compatible string

> +
> + crg: crg@12010000 {
> + compatible = "hisilicon,hi3519-crg";


what is a "crg"? Is there a standard name for these?

Arnd

2015-12-04 02:30:30

by Jiancheng Xue

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

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

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

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

>> +
>> +#include "skeleton.dtsi"
>> +#include <dt-bindings/clock/hi3519-clock.h>
>> +/ {
>> + aliases {
>> + serial0 = &uart0;
>> + };
>
> Move this into the .dts file.

OK. Thank you.

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

OK.

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

OK.

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

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

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

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

what about writing like this?

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

>
> Arnd
>
> .
>

Jiancheng
.

2015-12-04 10:50:48

by Arnd Bergmann

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

On Friday 04 December 2015 10:27:58 xuejiancheng wrote:
> >
> >> + sysctrl: system-controller@12020000 {
> >> + compatible = "hisilicon,sysctrl";
> >> + reg = <0x12020000 0x1000>;
> >> + reboot-offset = <0x4>;
> >> + };
> >
> > Is this one identical to the one in hip04?
> >
> > If not, pick a new unique compatible string
>
> Yes. It's compatible with the one in hip04.

Ok, we should add a compatible string for that then, as the hip04 apparently
has a slightly different layout from hip01.

Can you add a separate patch to clarify the existing hisilicon,sysctrl nodes?

It look like hi3620 accidentally uses the same string as hip04 already
but is actually a bit different.

Maybe split out the sysctrl binding from
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt, as it has
you already have a couple of those, and it's not clear how they relate
to one another.

If we introduce a string for all hip04 compatible sysctrl devices, we should
document that and use it consistently, so hi3519 becomes

compatible = "hisilicon,hi3519-sysctrl", "hisilicon,hip04-sysctrl", "hisilicon,sysctrl";

but I'd clarify in the binding documentation that "hisilicon,sysctrl" should
only be used for hip04 and hi3519 but not the others.

As this seems to be a standard part, we can also think about making a
high-level driver for in in drivers/soc rather than relying on the syscon
driver which we tend to use more for one-off devices with random register
layouts.

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

Ok, that's better.

Arnd

2015-12-07 06:49:42

by Jiancheng Xue

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


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

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

2015-12-08 03:35:38

by zhangfei

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



On 12/07/2015 02:37 PM, xuejiancheng wrote:

>> As this seems to be a standard part, we can also think about making a
>> high-level driver for in in drivers/soc rather than relying on the syscon
>> driver which we tend to use more for one-off devices with random register
>> layouts.
>>
> Sorry. I didn't understand your meaning well and maybe I gave you a wrong description.
> Please allow me to clarify it again.
> The "sysctrl" nodes here is just used for the "reboot" function. It is corresponding to
> the driver "drivers/power/reset/hisi-reboot.c". The compatible string in the driver is
> "hisilicon,sysctrl".


Pls try use drivers/power/reset/syscon-reboot.c

Thanks

2015-12-08 03:57:26

by Jiancheng Xue

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



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

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

Thank you.

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

2015-12-09 15:32:53

by Arnd Bergmann

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

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

This is not what I meant. You have to use "syscon" as the most generic
"compatible" value here, but should add a machine specific string
as a more specific one. "hisilicon,sysctrl" is not appropriate because
it does not identify the IP block uniquely, you can only use that
in combination with another more specific string.

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

Arnd

2015-12-10 06:35:08

by Jiancheng Xue

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



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

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

Thank you!

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

2015-12-10 08:06:07

by Arnd Bergmann

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

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

Why hisilicon,hi3519-syscon instead of hisilicon,hi3519-sysctrl?

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

Arnd

2015-12-10 09:05:27

by Jiancheng Xue

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



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

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

It is OK to use hisilicon,hi3519-sysctrl.

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

> Arnd
>
> .
>

2015-12-10 09:15:11

by Arnd Bergmann

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

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

"syscon" is the generic name for the framework in the kernel.
This has nothing to do with how chip designers call their devices,
and the DT should always match what the datasheet says, not what
a random operating system calls things, even if it's the only OS
you care about.

Arnd