| Message ID | 1428916660-25910-4-git-send-email-bintian.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 13 April 2015 17:17:37 Bintian Wang wrote: > +- compatible: the compatible should be one of the following strings to > + indicate the clock controller functionality. > + > + - "hisilicon,aoctrl" > + - "hisilicon,sysctrl" > + - "hisilicon,mediactrl" > + - "hisilicon,pmctrl" > + > These ones already have bindings, you can't reuse the strings. Please work with someone in hisilicon to set up a registry of device names so you can avoid conflicts in the future. Arnd
Hello Arnd, On 2015/4/13 23:32, Arnd Bergmann wrote: > On Monday 13 April 2015 17:17:37 Bintian Wang wrote: >> +- compatible: the compatible should be one of the following strings to >> + indicate the clock controller functionality. >> + >> + - "hisilicon,aoctrl" >> + - "hisilicon,sysctrl" >> + - "hisilicon,mediactrl" >> + - "hisilicon,pmctrl" >> + >> > > These ones already have bindings, you can't reuse the strings. > Please work with someone in hisilicon to set up a registry of > device names so you can avoid conflicts in the future. All the clock registers are under above four system controllers, discussed with Mark and Haojian two months ago, I think using above same four binding strings is enough for clk module. On second thoughts, there really some problems for future hisilicon code upstream, how about change back to the first version of this patch set, just like following: + sys_ctrl: sys_ctrl { + compatible = "hisilicon,sysctrl", "syscon"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0x0 0xf7030000 0x0 0x2000>; + ranges = <0 0x0 0xf7030000 0x2000>; + + clock_sys: clock1@0 { + compatible = "hisilicon,hi6220-clock-sys"; + reg = <0 0x1000>; + #clock-cells = <1>; + }; + }; Thanks, Bintian > > Arnd > > . >
On Tuesday 14 April 2015 11:35:26 Bintian wrote: > Hello Arnd, > > On 2015/4/13 23:32, Arnd Bergmann wrote: > > On Monday 13 April 2015 17:17:37 Bintian Wang wrote: > >> +- compatible: the compatible should be one of the following strings to > >> + indicate the clock controller functionality. > >> + > >> + - "hisilicon,aoctrl" > >> + - "hisilicon,sysctrl" > >> + - "hisilicon,mediactrl" > >> + - "hisilicon,pmctrl" > >> + > >> > > > > These ones already have bindings, you can't reuse the strings. > > Please work with someone in hisilicon to set up a registry of > > device names so you can avoid conflicts in the future. > All the clock registers are under above four system controllers, > discussed with Mark and Haojian two months ago, I think using above > same four binding strings is enough for clk module. > On second thoughts, there really some problems for future hisilicon > code upstream, how about change back to the first version of this > patch set, just like following: > + sys_ctrl: sys_ctrl { > + compatible = "hisilicon,sysctrl", "syscon"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x0 0xf7030000 0x0 0x2000>; > + ranges = <0 0x0 0xf7030000 0x2000>; > + > + clock_sys: clock1@0 { > + compatible = "hisilicon,hi6220-clock-sys"; > + reg = <0 0x1000>; > + #clock-cells = <1>; > + }; > + }; Sub-nodes are fine, but you also can't have a device node that is just 'compatible = "hisilicon,sysctrl", "syscon";' when hisilicon,sysctrl can refer to multiple mutually incompatible controllers. The minimum fix would be to mandate the string to be compatible = "hisilicon,hi6220-sysctrl", "hisilicon,sysctrl", "syscon"; for this model, and use respective compatible strings for the other chips. For the documentation, I think it would make sense to move that description to "hisilicon,sysctrl" and list all the variants of that component and the respective compatible strings there, rather than have one document per chip and list multiple components in it. Arnd
Hello Arnd, On 2015/4/14 18:19, Arnd Bergmann wrote: > On Tuesday 14 April 2015 11:35:26 Bintian wrote: >> Hello Arnd, >> >> On 2015/4/13 23:32, Arnd Bergmann wrote: >>> On Monday 13 April 2015 17:17:37 Bintian Wang wrote: >>>> +- compatible: the compatible should be one of the following strings to >>>> + indicate the clock controller functionality. >>>> + >>>> + - "hisilicon,aoctrl" >>>> + - "hisilicon,sysctrl" >>>> + - "hisilicon,mediactrl" >>>> + - "hisilicon,pmctrl" >>>> + >>>> >>> >>> These ones already have bindings, you can't reuse the strings. >>> Please work with someone in hisilicon to set up a registry of >>> device names so you can avoid conflicts in the future. >> All the clock registers are under above four system controllers, >> discussed with Mark and Haojian two months ago, I think using above >> same four binding strings is enough for clk module. >> On second thoughts, there really some problems for future hisilicon >> code upstream, how about change back to the first version of this >> patch set, just like following: >> + sys_ctrl: sys_ctrl { >> + compatible = "hisilicon,sysctrl", "syscon"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + reg = <0x0 0xf7030000 0x0 0x2000>; >> + ranges = <0 0x0 0xf7030000 0x2000>; >> + >> + clock_sys: clock1@0 { >> + compatible = "hisilicon,hi6220-clock-sys"; >> + reg = <0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + }; > > Sub-nodes are fine, but you also can't have a device node that > is just 'compatible = "hisilicon,sysctrl", "syscon";' when > hisilicon,sysctrl can refer to multiple mutually incompatible > controllers. > > The minimum fix would be to mandate the string to be > compatible = "hisilicon,hi6220-sysctrl", "hisilicon,sysctrl", "syscon"; > for this model, and use respective compatible strings for the other > chips. It's really a smart fix! For the four system controllers, how about change the following strings: + - "hisilicon,aoctrl" + - "hisilicon,sysctrl" + - "hisilicon,mediactrl" + - "hisilicon,pmctrl" to + - "hisilicon,hi6220-aoctrl" + - "hisilicon,hi6220-sysctrl" + - "hisilicon,hi6220-mediactrl" + - "hisilicon,hi6220-pmctrl" ? and I also use "hisilicon,hi6220-xxxx" for hi6220 clk driver directly ? > > For the documentation, I think it would make sense to move that > description to "hisilicon,sysctrl" and list all the variants of that > component and the respective compatible strings there, rather than > have one document per chip and list multiple components in it. Sure, Thanks, Bintian > > Arnd > > . >
On Tuesday 14 April 2015 20:37:11 Bintian wrote: > It's really a smart fix! > > For the four system controllers, how about change the following strings: > + - "hisilicon,aoctrl" > + - "hisilicon,sysctrl" > + - "hisilicon,mediactrl" > + - "hisilicon,pmctrl" > > to > + - "hisilicon,hi6220-aoctrl" > + - "hisilicon,hi6220-sysctrl" > + - "hisilicon,hi6220-mediactrl" > + - "hisilicon,hi6220-pmctrl" Yes, that works. > and I also use "hisilicon,hi6220-xxxx" for hi6220 clk driver directly ? Seems fine to me, though I'd have to look at the bigger picture to see any potential problems. At the moment, I don't see any. Arnd
Hello Arnd, 2015-04-14 21:20 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: > On Tuesday 14 April 2015 20:37:11 Bintian wrote: >> It's really a smart fix! >> >> For the four system controllers, how about change the following strings: >> + - "hisilicon,aoctrl" >> + - "hisilicon,sysctrl" >> + - "hisilicon,mediactrl" >> + - "hisilicon,pmctrl" >> >> to >> + - "hisilicon,hi6220-aoctrl" >> + - "hisilicon,hi6220-sysctrl" >> + - "hisilicon,hi6220-mediactrl" >> + - "hisilicon,hi6220-pmctrl" > > Yes, that works. Thanks, will fix in next version. > >> and I also use "hisilicon,hi6220-xxxx" for hi6220 clk driver directly ? > > Seems fine to me, though I'd have to look at the bigger picture to see > any potential problems. At the moment, I don't see any. Sure, please also help to review the 6/6 patch of this patch set, many thanks! BR, Bintian > > Arnd > -- > 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/
diff --git a/Documentation/devicetree/bindings/clock/hi6220-clock.txt b/Documentation/devicetree/bindings/clock/hi6220-clock.txt new file mode 100644 index 0000000..83cc7c7 --- /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,aoctrl" + - "hisilicon,sysctrl" + - "hisilicon,mediactrl" + - "hisilicon,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,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>.