| Message ID | 529EA80A.60801@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 04, 2013 at 03:56:58AM +0000, Alex Elder wrote: > Document the device tree binding for Broadcom Kona architecture > clock control units and clocks. Kona device nodes are represented > with compatible strings having "bcm11351" in their name. > > Kona clocks are managed by "clock control units" (CCUs). Each CCU > has a device tree node, and within that node are defined the names > of the clocks provided by the CCU. > > The BCM281xx family of SoCs use Kona CCUs and clocks. > > Signed-off-by: Alex Elder <elder@linaro.org> > Reviewed-by: Matt Porter <matt.porter@linaro.org> > Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > --- > .../devicetree/bindings/clock/bcm-kona-clock.txt | 95 > ++++++++++++++++++++ > 1 file changed, 95 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/clock/bcm-kona-clock.txt > > diff --git a/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt > b/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt > new file mode 100644 > index 0000000..0cafd6a > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt > @@ -0,0 +1,95 @@ > +Broadcom Kona Family Clocks > + > +This binding is associated with Broadcom SoCs having "Kona" style > +clock control units (CCUs). A CCU is a clock provider that manages > +a set of clock signals. Each CCU is represented by a node in the > +device tree. > + > +This binding uses the common clock binding: > + Documentation/devicetree/bindings/clock/clock-bindings.txt > + > +Many source clocks are described using the "fixed-clock" binding: > + Documentation/devicetree/bindings/clock/fixed-clock.txt Is this necessary? While this describes the device it doesn't describe this binding. > + > +Required properties: > +- compatible > + Shall have a value "brcm,bcm11351-<which>-ccu", where > + <which> identifies the particular CCU (see below). It would be nice to have a full list here, as that makes finding the file easier. Something like: - compatible: should contain one of: * "brcm,bcm11351-root-ccu" * "brcm,bcm11351-aon-ccu" * "brcm,bcm11351-hub-ccu" * "brcm,bcm11351-master-ccu" The differences between these are described in greater detail below. > +- reg > + Shall define the base and range of the address space > + containing clock control registers > +- #clock-cells > + Shall have the value <1> How about: - #clock-cells: Should be <1>. The permitted clock-specifier values are defined below. > +- clock-output-names > + Shall be an ordered list of strings defining the names of > + the clocks provided by the CCU. > + > +Clock consumers refer to a particular clock supplied by a CCU using > +a phandle and specifier pair, using the phandle for the CCU device > +tree node and the clock's symbolic specifier. The clock specifier > +is a CCU-unique 0-based index value. This is redundant given the common clock binding and the description of #clock-cells above. > + > + > +BCM281XX family SoCs use Kona CCUs. The following table defines > +the set of CCUs and clock specifiers for BCM281XX clocks. The > +compatible string for the CCU uses the name in the "CCU" column > +below as it's <which> value. > + > + CCU Clock Type Index Specifier > + --- ----- ---- ----- --------- > + root frac_1m peri 0 BCM281XX_ROOT_CCU_FRAC_1M > + > + aon hub_timer peri 0 BCM281XX_AON_CCU_HUB_TIMER > + aon pmu_bsc peri 1 BCM281XX_AON_CCU_PMU_BSC > + aon pmu_bsc_var peri 2 BCM281XX_AON_CCU_PMU_BSC_VAR > + > + hub tmon_1m peri 0 BCM281XX_HUB_CCU_TMON_1M > + > + master sdio1 peri 0 BCM281XX_MASTER_CCU_SDIO1 > + master sdio2 peri 1 BCM281XX_MASTER_CCU_SDIO2 > + master sdio3 peri 2 BCM281XX_MASTER_CCU_SDIO3 > + master sdio4 peri 3 BCM281XX_MASTER_CCU_SDIO4 > + master dmac peri 4 BCM281XX_MASTER_CCU_DMAC > + master usb_ic peri 5 BCM281XX_MASTER_CCU_USB_IC > + master hsic2_48m peri 6 BCM281XX_MASTER_CCU_HSIC_48M > + master hsic2_12m peri 7 BCM281XX_MASTER_CCU_HSIC_12M > + > + slave uartb peri 0 BCM281XX_SLAVE_CCU_UARTB > + slave uartb2 peri 1 BCM281XX_SLAVE_CCU_UARTB2 > + slave uartb3 peri 2 BCM281XX_SLAVE_CCU_UARTB3 > + slave uartb4 peri 3 BCM281XX_SLAVE_CCU_UARTB4 > + slave ssp0 peri 4 BCM281XX_SLAVE_CCU_SSP0 > + slave ssp2 peri 5 BCM281XX_SLAVE_CCU_SSP2 > + slave bsc1 peri 6 BCM281XX_SLAVE_CCU_BSC1 > + slave bsc2 peri 7 BCM281XX_SLAVE_CCU_BSC2 > + slave bsc3 peri 8 BCM281XX_SLAVE_CCU_BSC3 > + slave pwm peri 9 BCM281XX_SLAVE_CCU_PWM I guess the Specifier column is referring to some defines in a header file? It would be good to mention which header file these are in. The Index is also a specifier, it just happens to not be symbolic. > + > + > +Device tree example: > + > + clocks { > + slave_ccu: slave_ccu { > + compatible = "brcm,bcm11351-slave-ccu"; > + reg = <0x3e011000 0x0f00>; > + #clock-cells = <1>; > + clock-output-names = "uartb", > + "uartb2", > + "uartb3", > + "uartb4"; > + }; > + ref_crystal_clk: ref_crystal { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <26000000>; > + }; > + }; This is wrong, as the clocks container node us not defined as any type of bus, and does not have the requisite #address-cells and #size-cells. Really this should _not_ be probed, as the kernel does not know anything about the clocks node. It could be some non-MMIO bus that it doesn't understand, or one which requires other clocks. I think the current way that we probe clocks is somewhat broken in this regard. There's no reason to put the clocks in this container at all; just put them under the root. If you really want to use a container, please at least use a simple-bus with the requisite #address-cells, #size-cells, and ranges properties. Thanks, Mark.
On 12/04/2013 03:39 AM, Mark Rutland wrote: > On Wed, Dec 04, 2013 at 03:56:58AM +0000, Alex Elder wrote: >> Document the device tree binding for Broadcom Kona architecture >> clock control units and clocks. Kona device nodes are represented >> with compatible strings having "bcm11351" in their name. >> >> Kona clocks are managed by "clock control units" (CCUs). Each CCU >> has a device tree node, and within that node are defined the names >> of the clocks provided by the CCU. >> >> The BCM281xx family of SoCs use Kona CCUs and clocks. >> >> Signed-off-by: Alex Elder <elder@linaro.org> >> Reviewed-by: Matt Porter <matt.porter@linaro.org> >> Reviewed-by: Tim Kryger <tim.kryger@linaro.org> >> --- >> .../devicetree/bindings/clock/bcm-kona-clock.txt | 95 >> ++++++++++++++++++++ >> 1 file changed, 95 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/clock/bcm-kona-clock.txt >> >> diff --git a/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt >> b/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt >> new file mode 100644 >> index 0000000..0cafd6a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt >> @@ -0,0 +1,95 @@ >> +Broadcom Kona Family Clocks >> + >> +This binding is associated with Broadcom SoCs having "Kona" style >> +clock control units (CCUs). A CCU is a clock provider that manages >> +a set of clock signals. Each CCU is represented by a node in the >> +device tree. >> + >> +This binding uses the common clock binding: >> + Documentation/devicetree/bindings/clock/clock-bindings.txt >> + >> +Many source clocks are described using the "fixed-clock" binding: >> + Documentation/devicetree/bindings/clock/fixed-clock.txt > > Is this necessary? While this describes the device it doesn't describe > this binding. It's probably gratuitous. I will remove it. >> +Required properties: >> +- compatible >> + Shall have a value "brcm,bcm11351-<which>-ccu", where >> + <which> identifies the particular CCU (see below). > > It would be nice to have a full list here, as that makes finding the > file easier. Something like: > > - compatible: should contain one of: > * "brcm,bcm11351-root-ccu" > * "brcm,bcm11351-aon-ccu" > * "brcm,bcm11351-hub-ccu" > * "brcm,bcm11351-master-ccu" > The differences between these are described in greater detail below. I got this suggestion in internal review. I gambled, and lost. :) I'll add the list as you suggest. >> +- reg >> + Shall define the base and range of the address space >> + containing clock control registers >> +- #clock-cells >> + Shall have the value <1> > > How about: > > - #clock-cells: Should be <1>. The permitted clock-specifier values are > defined below. OK. >> +- clock-output-names >> + Shall be an ordered list of strings defining the names of >> + the clocks provided by the CCU. >> + >> +Clock consumers refer to a particular clock supplied by a CCU using >> +a phandle and specifier pair, using the phandle for the CCU device >> +tree node and the clock's symbolic specifier. The clock specifier >> +is a CCU-unique 0-based index value. > > This is redundant given the common clock binding and the description of > #clock-cells above. OK. I'll delete this paragraph. >> + >> + >> +BCM281XX family SoCs use Kona CCUs. The following table defines >> +the set of CCUs and clock specifiers for BCM281XX clocks. The >> +compatible string for the CCU uses the name in the "CCU" column >> +below as it's <which> value. >> + >> + CCU Clock Type Index Specifier >> + --- ----- ---- ----- --------- >> + root frac_1m peri 0 BCM281XX_ROOT_CCU_FRAC_1M >> + >> + aon hub_timer peri 0 BCM281XX_AON_CCU_HUB_TIMER >> + aon pmu_bsc peri 1 BCM281XX_AON_CCU_PMU_BSC >> + aon pmu_bsc_var peri 2 BCM281XX_AON_CCU_PMU_BSC_VAR >> + >> + hub tmon_1m peri 0 BCM281XX_HUB_CCU_TMON_1M >> + >> + master sdio1 peri 0 BCM281XX_MASTER_CCU_SDIO1 >> + master sdio2 peri 1 BCM281XX_MASTER_CCU_SDIO2 >> + master sdio3 peri 2 BCM281XX_MASTER_CCU_SDIO3 >> + master sdio4 peri 3 BCM281XX_MASTER_CCU_SDIO4 >> + master dmac peri 4 BCM281XX_MASTER_CCU_DMAC >> + master usb_ic peri 5 BCM281XX_MASTER_CCU_USB_IC >> + master hsic2_48m peri 6 BCM281XX_MASTER_CCU_HSIC_48M >> + master hsic2_12m peri 7 BCM281XX_MASTER_CCU_HSIC_12M >> + >> + slave uartb peri 0 BCM281XX_SLAVE_CCU_UARTB >> + slave uartb2 peri 1 BCM281XX_SLAVE_CCU_UARTB2 >> + slave uartb3 peri 2 BCM281XX_SLAVE_CCU_UARTB3 >> + slave uartb4 peri 3 BCM281XX_SLAVE_CCU_UARTB4 >> + slave ssp0 peri 4 BCM281XX_SLAVE_CCU_SSP0 >> + slave ssp2 peri 5 BCM281XX_SLAVE_CCU_SSP2 >> + slave bsc1 peri 6 BCM281XX_SLAVE_CCU_BSC1 >> + slave bsc2 peri 7 BCM281XX_SLAVE_CCU_BSC2 >> + slave bsc3 peri 8 BCM281XX_SLAVE_CCU_BSC3 >> + slave pwm peri 9 BCM281XX_SLAVE_CCU_PWM > > I guess the Specifier column is referring to some defines in a header > file? It would be good to mention which header file these are in. Yes. I will add a reference to it. > The Index is also a specifier, it just happens to not be symbolic. Yes it is, but I was having trouble trying to describe them. "Symbolic specifier" got to be unwieldy. The paragraph that talked about the "0-based index value" (which I said I would delete) was an attempt to at least give it some sort of name. If someone has a suggestion for how best to describe this I'm totally open. >> + >> + >> +Device tree example: >> + >> + clocks { >> + slave_ccu: slave_ccu { >> + compatible = "brcm,bcm11351-slave-ccu"; >> + reg = <0x3e011000 0x0f00>; >> + #clock-cells = <1>; >> + clock-output-names = "uartb", >> + "uartb2", >> + "uartb3", >> + "uartb4"; >> + }; >> + ref_crystal_clk: ref_crystal { >> + #clock-cells = <0>; >> + compatible = "fixed-clock"; >> + clock-frequency = <26000000>; >> + }; >> + }; > > This is wrong, as the clocks container node us not defined as any type > of bus, and does not have the requisite #address-cells and #size-cells. Sorry, I didn't realize it was wrong. I intentionally used it simply for groiuping. > Really this should _not_ be probed, as the kernel does not know anything > about the clocks node. It could be some non-MMIO bus that it doesn't > understand, or one which requires other clocks. > > I think the current way that we probe clocks is somewhat broken in this > regard. > > There's no reason to put the clocks in this container at all; just put > them under the root. If you really want to use a container, please at > least use a simple-bus with the requisite #address-cells, #size-cells, > and ranges properties. No, I'll just pull them all out of the container. Thank you very much for the quick review. (And on to the next one...) -Alex > > Thanks, > Mark. >
On 12/04/2013 06:45 AM, Alex Elder wrote: >>> +Device tree example: >>> >> + >>> >> + clocks { >>> >> + slave_ccu: slave_ccu { >>> >> + compatible = "brcm,bcm11351-slave-ccu"; >>> >> + reg = <0x3e011000 0x0f00>; >>> >> + #clock-cells = <1>; >>> >> + clock-output-names = "uartb", >>> >> + "uartb2", >>> >> + "uartb3", >>> >> + "uartb4"; >>> >> + }; >>> >> + ref_crystal_clk: ref_crystal { >>> >> + #clock-cells = <0>; >>> >> + compatible = "fixed-clock"; >>> >> + clock-frequency = <26000000>; >>> >> + }; >>> >> + }; >> > >> > This is wrong, as the clocks container node us not defined as any type >> > of bus, and does not have the requisite #address-cells and #size-cells. > Sorry, I didn't realize it was wrong. I intentionally used > it simply for groiuping. I should have looked at my actual dtsi file before responding. It turns out that what was shown there was not complete. The dtsi file in fact does contain #address-cells and #size-cells (and a direct-mapping "ranges" property). I have removed the enclosing "clocks" node and its braces in the document. I have not removed the node from the dtsi file. -Alex
diff --git a/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt b/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt new file mode 100644 index 0000000..0cafd6a --- /dev/null +++ b/Documentation/devicetree/bindings/clock/bcm-kona-clock.txt @@ -0,0 +1,95 @@ +Broadcom Kona Family Clocks + +This binding is associated with Broadcom SoCs having "Kona" style +clock control units (CCUs). A CCU is a clock provider that manages +a set of clock signals. Each CCU is represented by a node in the +device tree. + +This binding uses the common clock binding: + Documentation/devicetree/bindings/clock/clock-bindings.txt + +Many source clocks are described using the "fixed-clock" binding: + Documentation/devicetree/bindings/clock/fixed-clock.txt + +Required properties: +- compatible + Shall have a value "brcm,bcm11351-<which>-ccu", where + <which> identifies the particular CCU (see below). +- reg + Shall define the base and range of the address space + containing clock control registers +- #clock-cells + Shall have the value <1> +- clock-output-names + Shall be an ordered list of strings defining the names of + the clocks provided by the CCU. + +Clock consumers refer to a particular clock supplied by a CCU using +a phandle and specifier pair, using the phandle for the CCU device +tree node and the clock's symbolic specifier. The clock specifier +is a CCU-unique 0-based index value. + + +BCM281XX family SoCs use Kona CCUs. The following table defines +the set of CCUs and clock specifiers for BCM281XX clocks. The +compatible string for the CCU uses the name in the "CCU" column +below as it's <which> value. + + CCU Clock Type Index Specifier + --- ----- ---- ----- --------- + root frac_1m peri 0 BCM281XX_ROOT_CCU_FRAC_1M + + aon hub_timer peri 0 BCM281XX_AON_CCU_HUB_TIMER + aon pmu_bsc peri 1 BCM281XX_AON_CCU_PMU_BSC + aon pmu_bsc_var peri 2 BCM281XX_AON_CCU_PMU_BSC_VAR + + hub tmon_1m peri 0 BCM281XX_HUB_CCU_TMON_1M + + master sdio1 peri 0 BCM281XX_MASTER_CCU_SDIO1 + master sdio2 peri 1 BCM281XX_MASTER_CCU_SDIO2 + master sdio3 peri 2 BCM281XX_MASTER_CCU_SDIO3 + master sdio4 peri 3 BCM281XX_MASTER_CCU_SDIO4 + master dmac peri 4 BCM281XX_MASTER_CCU_DMAC + master usb_ic peri 5 BCM281XX_MASTER_CCU_USB_IC + master hsic2_48m peri 6 BCM281XX_MASTER_CCU_HSIC_48M + master hsic2_12m peri 7 BCM281XX_MASTER_CCU_HSIC_12M + + slave uartb peri 0 BCM281XX_SLAVE_CCU_UARTB + slave uartb2 peri 1 BCM281XX_SLAVE_CCU_UARTB2 + slave uartb3 peri 2 BCM281XX_SLAVE_CCU_UARTB3 + slave uartb4 peri 3 BCM281XX_SLAVE_CCU_UARTB4 + slave ssp0 peri 4 BCM281XX_SLAVE_CCU_SSP0 + slave ssp2 peri 5 BCM281XX_SLAVE_CCU_SSP2 + slave bsc1 peri 6 BCM281XX_SLAVE_CCU_BSC1 + slave bsc2 peri 7 BCM281XX_SLAVE_CCU_BSC2 + slave bsc3 peri 8 BCM281XX_SLAVE_CCU_BSC3 + slave pwm peri 9 BCM281XX_SLAVE_CCU_PWM + + +Device tree example: + + clocks { + slave_ccu: slave_ccu { + compatible = "brcm,bcm11351-slave-ccu"; + reg = <0x3e011000 0x0f00>; + #clock-cells = <1>; + clock-output-names = "uartb", + "uartb2", + "uartb3", + "uartb4"; + }; + ref_crystal_clk: ref_crystal { + #clock-cells = <0>; + compatible = "fixed-clock"; + clock-frequency = <26000000>; + }; + }; + uart@3e002000 { + compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart"; + status = "disabled"; + reg = <0x3e002000 0x1000>; + clocks = <&slave_ccu BCM281XX_SLAVE_CCU_UARTB3>; + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>; + reg-shift = <2>; + reg-io-width = <4>; + };