| Message ID | 20160826153725.11629-25-paul.burton@imgtec.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 08/26, Paul Burton wrote: > diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt > new file mode 100644 > index 0000000..c01ea60 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt > @@ -0,0 +1,27 @@ > +Binding for Imagination Technologies MIPS Boston clock sources. > + > +This binding uses the common clock binding[1]. > + > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > + > +Required properties: > +- compatible : Should be "img,boston-clock". > +- #clock-cells : Should be set to 1. > + Values available for clock consumers can be found in the header file: > + <dt-bindings/clock/boston-clock.h> > +- regmap : Phandle to the Boston platform register system controller. > + This should contain a phandle to the system controller node covering the > + platform registers provided by the Boston board. > + > +Example: > + > + clk_boston: clock { > + compatible = "img,boston-clock"; > + #clock-cells = <1>; > + regmap = <&plat_regs>; Isn't syscon more standard than regmap as the property name? Is there a binding for the plat_regs device? Is there any reason the clks can't be populated in that syscon driver? > + }; > + > + uart0: uart@17ffe000 { > + /* ... */ > + clocks = <&clk_boston BOSTON_CLK_SYS>; > + };
On 26/08/16 18:44, Stephen Boyd wrote: > On 08/26, Paul Burton wrote: >> diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt >> new file mode 100644 >> index 0000000..c01ea60 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt >> @@ -0,0 +1,27 @@ >> +Binding for Imagination Technologies MIPS Boston clock sources. >> + >> +This binding uses the common clock binding[1]. >> + >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt >> + >> +Required properties: >> +- compatible : Should be "img,boston-clock". >> +- #clock-cells : Should be set to 1. >> + Values available for clock consumers can be found in the header file: >> + <dt-bindings/clock/boston-clock.h> >> +- regmap : Phandle to the Boston platform register system controller. >> + This should contain a phandle to the system controller node covering the >> + platform registers provided by the Boston board. >> + >> +Example: >> + >> + clk_boston: clock { >> + compatible = "img,boston-clock"; >> + #clock-cells = <1>; >> + regmap = <&plat_regs>; > > Isn't syscon more standard than regmap as the property name? Is > there a binding for the plat_regs device? Is there any reason the > clks can't be populated in that syscon driver? Hi Stephen, The plat_regs device doesn't have a custom driver, it simply makes use of the generic "syscon" driver which can provide a regmap. It would be possible to register the clocks from a register for the plat_regs device, but I don't think it would make much sense. The platform registers in question are essentially just a convenient place where various bits of information about the system are exposed, including the clock frequencies but also other bits & pieces like connectivity of PCIe controllers or I/O coherence units, the RTL revision of the CPU or the wrapper RTL that runs on this FPGA-based board, a register that allows for resetting the board, etc. It's not a single piece of hardware, more a dumping ground for miscellanea. So in my opinion using the syscon approach works best here, and drivers for well defined pieces of hardware or functionality can reference that syscon to retrieve the regmap. As for whether "syscon" is a more standard property name than "regmap", both seem to be used based on a grep of Documentation/devicetree/bindings/. I believe I picked up use of "regmap" from the generic syscon-poweroff & syscon-reboot drivers, which both use "regmap" as a property name. Thanks, Paul > >> + }; >> + >> + uart0: uart@17ffe000 { >> + /* ... */ >> + clocks = <&clk_boston BOSTON_CLK_SYS>; >> + }; > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 30, 2016 at 04:53:01PM +0100, Paul Burton wrote: > On 26/08/16 18:44, Stephen Boyd wrote: > > On 08/26, Paul Burton wrote: > >> diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt > >> new file mode 100644 > >> index 0000000..c01ea60 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt > >> @@ -0,0 +1,27 @@ > >> +Binding for Imagination Technologies MIPS Boston clock sources. > >> + > >> +This binding uses the common clock binding[1]. > >> + > >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > >> + > >> +Required properties: > >> +- compatible : Should be "img,boston-clock". > >> +- #clock-cells : Should be set to 1. > >> + Values available for clock consumers can be found in the header file: > >> + <dt-bindings/clock/boston-clock.h> > >> +- regmap : Phandle to the Boston platform register system controller. > >> + This should contain a phandle to the system controller node covering the > >> + platform registers provided by the Boston board. > >> + > >> +Example: > >> + > >> + clk_boston: clock { > >> + compatible = "img,boston-clock"; > >> + #clock-cells = <1>; > >> + regmap = <&plat_regs>; > > > > Isn't syscon more standard than regmap as the property name? Is > > there a binding for the plat_regs device? Is there any reason the > > clks can't be populated in that syscon driver? > > Hi Stephen, > > The plat_regs device doesn't have a custom driver, it simply makes use > of the generic "syscon" driver which can provide a regmap. > > It would be possible to register the clocks from a register for the > plat_regs device, but I don't think it would make much sense. The > platform registers in question are essentially just a convenient place > where various bits of information about the system are exposed, > including the clock frequencies but also other bits & pieces like > connectivity of PCIe controllers or I/O coherence units, the RTL > revision of the CPU or the wrapper RTL that runs on this FPGA-based > board, a register that allows for resetting the board, etc. It's not a > single piece of hardware, more a dumping ground for miscellanea. So in > my opinion using the syscon approach works best here, and drivers for > well defined pieces of hardware or functionality can reference that > syscon to retrieve the regmap. That is all quite common for any SoC. Whether it's 2 nodes or 2 drivers are independent questions. You can easily have 1 node and 2 drivers. The decision factor is really how many registers we're dealing with. We don't want to end up with a node per register or register field. That's too fine grained. > As for whether "syscon" is a more standard property name than "regmap", > both seem to be used based on a grep of > Documentation/devicetree/bindings/. I believe I picked up use of > "regmap" from the generic syscon-poweroff & syscon-reboot drivers, which > both use "regmap" as a property name. syscon is much more common. Avoid the phandle altogether and make this a child node. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/09/16 13:54, Rob Herring wrote: > On Tue, Aug 30, 2016 at 04:53:01PM +0100, Paul Burton wrote: >> On 26/08/16 18:44, Stephen Boyd wrote: >>> On 08/26, Paul Burton wrote: >>>> diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt >>>> new file mode 100644 >>>> index 0000000..c01ea60 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt >>>> @@ -0,0 +1,27 @@ >>>> +Binding for Imagination Technologies MIPS Boston clock sources. >>>> + >>>> +This binding uses the common clock binding[1]. >>>> + >>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt >>>> + >>>> +Required properties: >>>> +- compatible : Should be "img,boston-clock". >>>> +- #clock-cells : Should be set to 1. >>>> + Values available for clock consumers can be found in the header file: >>>> + <dt-bindings/clock/boston-clock.h> >>>> +- regmap : Phandle to the Boston platform register system controller. >>>> + This should contain a phandle to the system controller node covering the >>>> + platform registers provided by the Boston board. >>>> + >>>> +Example: >>>> + >>>> + clk_boston: clock { >>>> + compatible = "img,boston-clock"; >>>> + #clock-cells = <1>; >>>> + regmap = <&plat_regs>; >>> >>> Isn't syscon more standard than regmap as the property name? Is >>> there a binding for the plat_regs device? Is there any reason the >>> clks can't be populated in that syscon driver? >> >> Hi Stephen, >> >> The plat_regs device doesn't have a custom driver, it simply makes use >> of the generic "syscon" driver which can provide a regmap. >> >> It would be possible to register the clocks from a register for the >> plat_regs device, but I don't think it would make much sense. The >> platform registers in question are essentially just a convenient place >> where various bits of information about the system are exposed, >> including the clock frequencies but also other bits & pieces like >> connectivity of PCIe controllers or I/O coherence units, the RTL >> revision of the CPU or the wrapper RTL that runs on this FPGA-based >> board, a register that allows for resetting the board, etc. It's not a >> single piece of hardware, more a dumping ground for miscellanea. So in >> my opinion using the syscon approach works best here, and drivers for >> well defined pieces of hardware or functionality can reference that >> syscon to retrieve the regmap. > > That is all quite common for any SoC. Whether it's 2 nodes or 2 drivers > are independent questions. You can easily have 1 node and 2 drivers. The > decision factor is really how many registers we're dealing with. We > don't want to end up with a node per register or register field. That's > too fine grained. Absolutely, I don't think we disagree there. >> As for whether "syscon" is a more standard property name than "regmap", >> both seem to be used based on a grep of >> Documentation/devicetree/bindings/. I believe I picked up use of >> "regmap" from the generic syscon-poweroff & syscon-reboot drivers, which >> both use "regmap" as a property name. > > syscon is much more common. > > Avoid the phandle altogether and make this a child node. I could do that, but it would feel rather odd to describe the clock hardware as a child of a bunch of miscellaneous registers that happen to expose some information about those clocks. Is that really what you prefer? I think as-is the DT is a better description of the hardware. Thanks, Paul > > Rob > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt new file mode 100644 index 0000000..c01ea60 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt @@ -0,0 +1,27 @@ +Binding for Imagination Technologies MIPS Boston clock sources. + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +Required properties: +- compatible : Should be "img,boston-clock". +- #clock-cells : Should be set to 1. + Values available for clock consumers can be found in the header file: + <dt-bindings/clock/boston-clock.h> +- regmap : Phandle to the Boston platform register system controller. + This should contain a phandle to the system controller node covering the + platform registers provided by the Boston board. + +Example: + + clk_boston: clock { + compatible = "img,boston-clock"; + #clock-cells = <1>; + regmap = <&plat_regs>; + }; + + uart0: uart@17ffe000 { + /* ... */ + clocks = <&clk_boston BOSTON_CLK_SYS>; + }; diff --git a/include/dt-bindings/clock/boston-clock.h b/include/dt-bindings/clock/boston-clock.h new file mode 100644 index 0000000..25f9cd2 --- /dev/null +++ b/include/dt-bindings/clock/boston-clock.h @@ -0,0 +1,13 @@ +/* + * Copyright (C) 2016 Imagination Technologies + * + * SPDX-License-Identifier: GPL-2.0 + */ + +#ifndef __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__ +#define __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__ + +#define BOSTON_CLK_SYS 0 +#define BOSTON_CLK_CPU 1 + +#endif /* __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__ */
Add device tree binding documentation for the clocks provided by the MIPS Boston development board from Imagination Technologies, and a header file describing the available clocks for use by device trees & driver. Signed-off-by: Paul Burton <paul.burton@imgtec.com> --- .../devicetree/bindings/clock/img,boston-clock.txt | 27 ++++++++++++++++++++++ include/dt-bindings/clock/boston-clock.h | 13 +++++++++++ 2 files changed, 40 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/img,boston-clock.txt create mode 100644 include/dt-bindings/clock/boston-clock.h