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@xxxxxxxxxx>
>> Reviewed-by: Matt Porter <matt.porter@xxxxxxxxxx>
>> Reviewed-by: Tim Kryger <tim.kryger@xxxxxxxxxx>
>> ---
>> .../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.
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
