lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 14 May 2014 15:32:30 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	"Sebastian Hesselbarth" <sebastian.hesselbarth@...il.com>
Cc:	"Rob Herring" <robh+dt@...nel.org>,
	"Pawel Moll" <pawel.moll@....com>,
	"Mark Rutland" <mark.rutland@....com>,
	"Ian Campbell" <ijc+devicetree@...lion.org.uk>,
	"Kumar Gala" <galak@...eaurora.org>,
	"Randy Dunlap" <rdunlap@...radead.org>,
	"Alexandre Belloni" <alexandre.belloni@...e-electrons.com>,
	"Antoine Tenart" <antoine.tenart@...e-electrons.com>,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/8] clk: berlin: add clock binding docs for Marvell Berlin2 SoCs

Quoting Sebastian Hesselbarth (2014-05-11 13:24:35)
> This adds mandatory device tree binding documentation for the clock related
> IP found on Marvell Berlin2 (BG2, BG2CD, and BG2Q) SoCs.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
> ---
> Cc: Mike Turquette <mturquette@...aro.org>
> Cc: Rob Herring <robh+dt@...nel.org>
> Cc: Pawel Moll <pawel.moll@....com>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Ian Campbell <ijc+devicetree@...lion.org.uk>
> Cc: Kumar Gala <galak@...eaurora.org>
> Cc: Randy Dunlap <rdunlap@...radead.org>
> Cc: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
> Cc: Antoine Tenart <antoine.tenart@...e-electrons.com>
> Cc: devicetree@...r.kernel.org
> Cc: linux-doc@...r.kernel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-kernel@...r.kernel.org
> ---
>  .../devicetree/bindings/clock/berlin2-clock.txt    | 169 +++++++++++++++++++++
>  1 file changed, 169 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/berlin2-clock.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/berlin2-clock.txt b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
> new file mode 100644
> index 000000000000..3da87a488402
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
> @@ -0,0 +1,169 @@
> +* Marvell Berlin2 clock bindings
> +
> +Marvell Berlin2 (BG2, BG2CD, BG2Q) share the same IP for PLLs and clocks,
> +with some minor differences in features and register layout. The below
> +describes the individual clock related IP:
> +
> +* Audio/Video PLL
> +
> +The Audio/Video PLL (AVPLL) is a dual-VCO PLL with 8 channels each. Each
> +of the VCOs can sythesize a single VCO frequency based on a single input
> +reference clock. Each of the 8 channels then, can derive an output clock
> +from that VCO frequency by various dividers/multipliers.
> +
> +Required properties:
> +- compatible: shall be "marvell,berlin2-avpll"
> +- reg: address and length of the corresponding AVPLL registers
> +- #clock-cells: shall be set to 2
> +- clocks: single clock specifier referencing the AVPLL input clock
> +
> +To ease match-up with the desired AVPLL output clock, clock specifiers
> +referencing AVPLL clocks shall contain two cells. The first refers to
> +the VCO (0=AVPLL_A, 1=AVPLL_B) while the second refers to the corresponding
> +channel starting with 1. For example, to reference AVPLL_B3 the clock
> +specifier shall be: <&avpll 1 3>.
> +
> +Example:
> +
> +avpll: pll@...040 {
> +       compatible = "marvell,berlin2-avpll";
> +       #clock-cells = <2>;
> +       reg = <0xea0050 0x100>;
> +       clocks = <&refclk>;
> +};

Hi Sebastian,

Thanks for submitting the series. It looks good. I do have some comments
about the DT bindings though. I'm encouraging new bindings (and
especially new platforms or existing platforms that are only now
converting over to CCF) to not put their per-clock data into DTS. This
has scalability problems, is unpopular with the DT crowd and sometimes
makes it hard to do things like set CLK_SET_RATE_PARENT flags for
individual clocks.

The following is a copy/paste from an email I sent earlier today[1]. Of
course per-clock data makes great sense if you have an off-SoC clock
such as a fixed-rate oscillator (e.g. the fixed-clock binding). Let me
know what you think:

I assume the rest of your clocks are part of a clock generator IP block
inside of your chip. Have you looked at the QCOM binding? It is my
favorite binding these days. Here are some highlights:

See Documentation/devicetree/bindings/clock/qcom,gcc.txt.



>From arch/arm/boot/dts/qcom-msm8974.dtsi:

gcc: clock-controller@...00000 {
        compatible = "qcom,gcc-msm8974";
        #clock-cells = <1>;
        #reset-cells = <1>;
        reg = <0xfc400000 0x4000>;
};

...

serial@...1e000 {
        compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
        reg = <0xf991e000 0x1000>;
        interrupts = <0 108 0x0>;
        clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>, <&gcc GCC_BLSP1_AHB_CLK>;
        clock-names = "core", "iface";
};



>From drivers/clk/qcom/gcc-msm8974.c:

static struct clk_branch gcc_blsp1_uart2_apps_clk = {
        .halt_reg = 0x0704,
        .clkr = {
                .enable_reg = 0x0704,
                .enable_mask = BIT(0),
                .hw.init = &(struct clk_init_data){
                        .name = "gcc_blsp1_uart2_apps_clk",
                        .parent_names = (const char *[]){
                                "blsp1_uart2_apps_clk_src",
                        },
                        .num_parents = 1,
                        .flags = CLK_SET_RATE_PARENT,
                        .ops = &clk_branch2_ops,
                },
        },
};

Using this type of binding you only need to declare your clock generator
IP node in dts, and then define a mapping in the DT include chroot. Then
you can define your per-clock data inside of your clock driver instead
of putting all of the details inside of DT.

If you have a strong reason to do it the way that you originally posted
then let me know.

Regards,
Mike

[1] https://lkml.org/lkml/2014/5/14/598
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ